From a24a964c7990d9230f2a265a27759b9bfc873c38 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 13 Feb 2025 09:48:46 -0800
Subject: [PATCH 01/27] feat: Implement higher-level APIs for getting
 components from units

---
 .../apps/authoring/containers/api.py          | 69 ++++++++++++++++++-
 openedx_learning/apps/authoring/units/api.py  | 52 ++++++++++++++
 .../apps/authoring/units/test_api.py          | 15 ++--
 3 files changed, 129 insertions(+), 7 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 4feb3a5f..adaa01f6 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -4,18 +4,21 @@
 This module provides a set of functions to interact with the containers
 models in the Open edX Learning platform.
 """
+from dataclasses import dataclass
 
 from django.db.transaction import atomic
 from django.db.models import QuerySet
 
 from datetime import datetime
+
+from openedx_learning.apps.authoring.containers.models_mixin import ContainerEntityMixin, ContainerEntityVersionMixin
 from ..containers.models import (
     ContainerEntity,
     ContainerEntityVersion,
     EntityList,
     EntityListRow,
 )
-from ..publishing.models import PublishableEntity
+from ..publishing.models import PublishableEntity, PublishableEntityVersion
 from ..publishing import api as publishing_api
 
 
@@ -28,6 +31,9 @@
     "get_defined_list_rows_for_container_version",
     "get_initial_list_rows_for_container_version",
     "get_frozen_list_rows_for_container_version",
+    "ContainerEntityListEntry",
+    "get_entities_in_draft_container",
+    "get_entities_in_published_container",
 ]
 
 
@@ -471,3 +477,64 @@ def get_frozen_list_rows_for_container_version(
     if container_version.frozen_list is None:
         return QuerySet[EntityListRow]()
     return container_version.frozen_list.entitylistrow_set.all()
+
+
+@dataclass(frozen=True)
+class ContainerEntityListEntry:
+    """
+    Data about a single entity in a container, e.g. a component in a unit.
+    """
+    entity_version: PublishableEntityVersion
+    pinned: bool
+
+    @property
+    def entity(self):
+        return self.entity_version.entity
+
+
+def get_entities_in_draft_container(
+    container: ContainerEntity | ContainerEntityMixin,
+) -> list[ContainerEntityListEntry]:
+    """
+    Get the list of entities and their versions in the draft version of the
+    given container.
+    """
+    if isinstance(container, ContainerEntityMixin):
+        container = container.container_entity
+    assert isinstance(container, ContainerEntity)
+    entity_list = []
+    defined_list = container.versioning.draft.defined_list
+    for row in defined_list.entitylistrow_set.order_by("order_num"):
+        entity_list.append(ContainerEntityListEntry(
+            entity_version=row.draft_version or row.entity.draft.version,
+            pinned=row.draft_version is not None,
+        ))
+    return entity_list
+
+
+def get_entities_in_published_container(
+    container: ContainerEntity | ContainerEntityMixin,
+) -> list[ContainerEntityListEntry] | None:
+    """
+    Get the list of entities and their versions in the draft version of the
+    given container.
+    """
+    if isinstance(container, ContainerEntityMixin):
+        cev = container.container_entity.versioning.published
+    elif isinstance(container, ContainerEntity):
+        cev = container.versioning.published
+    if cev == None:
+        return None  # There is no published version of this container. Should this be an exception?
+    assert isinstance(cev, ContainerEntityVersion)
+    # TODO: do we ever need frozen_list? e.g. when accessing a historical version?
+    # Doesn't make a lot of sense when the versions of the container don't capture many of the changes to the contents,
+    # e.g. container version 1 had component version 1 through 50, and container version 2 had component versions 51
+    # through 100, what is the point of tracking whether container version 1 "should" show v1 or v50 when they're wildly
+    # different?
+    entity_list = []
+    for row in cev.defined_list.entitylistrow_set.order_by("order_num"):
+        entity_list.append(ContainerEntityListEntry(
+            entity_version=row.published_version or row.entity.published.version,
+            pinned=row.published_version is not None,
+        ))
+    return entity_list
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index c0a950b2..a42324fc 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -2,9 +2,11 @@
 
 This module provides functions to manage units.
 """
+from dataclasses import dataclass
 
 from django.db.transaction import atomic
 
+from openedx_learning.apps.authoring.components.models import ComponentVersion
 from openedx_learning.apps.authoring.containers.models import EntityListRow
 from ..publishing import api as publishing_api
 from ..containers import api as container_api
@@ -25,6 +27,9 @@
     "get_user_defined_list_in_unit_version",
     "get_initial_list_in_unit_version",
     "get_frozen_list_in_unit_version",
+    "UnitListEntry",
+    "get_components_in_draft_unit",
+    "get_components_in_published_unit",
 ]
 
 
@@ -216,3 +221,50 @@ def get_frozen_list_in_unit_version(unit_version_pk: int) -> list[int]:
     """
     unit_version = UnitVersion.objects.get(pk=unit_version_pk)
     return container_api.get_frozen_list_rows_for_container_version(unit_version.container_entity_version)
+
+
+@dataclass(frozen=True)
+class UnitListEntry:
+    """
+    Data about a single entity in a container, e.g. a component in a unit.
+    """
+    component_version: ComponentVersion
+    pinned: bool
+
+    @property
+    def component(self):
+        return self.component_version.component
+
+
+def get_components_in_draft_unit(
+    unit: Unit,
+) -> list[UnitListEntry]:
+    """
+    Get the list of entities and their versions in the draft version of the
+    given container.
+    """
+    assert isinstance(unit, Unit)
+    entity_list = []
+    for entry in container_api.get_entities_in_draft_container(unit):
+        # Convert from generic PublishableEntityVersion to ComponentVersion:
+        component_version = entry.entity_version.componentversion
+        assert isinstance(component_version, ComponentVersion)
+        entity_list.append(UnitListEntry(component_version=component_version, pinned=entry.pinned))
+    return entity_list
+
+
+def get_components_in_published_unit(
+    unit: Unit | UnitVersion,
+) -> list[UnitListEntry]:
+    """
+    Get the list of entities and their versions in the draft version of the
+    given container.
+    """
+    assert isinstance(unit, (Unit, UnitVersion))
+    entity_list = []
+    for entry in container_api.get_entities_in_published_container(unit):
+        # Convert from generic PublishableEntityVersion to ComponentVersion:
+        component_version = entry.entity_version.componentversion
+        assert isinstance(component_version, ComponentVersion)
+        entity_list.append(UnitListEntry(component_version=component_version, pinned=entry.pinned))
+    return entity_list
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index e09253c6..a805f6b6 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -33,13 +33,14 @@ def test_create_unit_with_content_instead_of_components(self):
         2. The unit is not created.
         """
 
-    def test_create_empty_first_unit_and_version(self):
+    def test_create_empty_unit_and_version(self):
         """Test creating a unit with no components.
 
         Expected results:
         1. A unit and unit version are created.
         2. The unit version number is 1.
-        3. The unit version is in the unit's versions.
+        3. The unit is a draft with unpublished changes.
+        4. There is no published version of the unit.
         """
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
@@ -51,6 +52,9 @@ def test_create_empty_first_unit_and_version(self):
         assert unit, unit_version
         assert unit_version.version_num == 1
         assert unit_version in unit.versioning.versions.all()
+        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.draft == unit_version
+        assert unit.versioning.published is None
 
     def test_create_next_unit_version_with_two_components(self):
         """Test creating a unit version with two components.
@@ -84,11 +88,10 @@ def test_create_next_unit_version_with_two_components(self):
         )
         assert unit_version_v2.version_num == 2
         assert unit_version_v2 in unit.versioning.versions.all()
-        publishable_entities_in_list = [
-            row.entity for row in authoring_api.get_user_defined_list_in_unit_version(unit_version_v2.pk)
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=self.component_1.versioning.draft, pinned=False),
+            authoring_api.UnitListEntry(component_version=self.component_2.versioning.draft, pinned=False),
         ]
-        assert self.component_1.publishable_entity in publishable_entities_in_list
-        assert self.component_2.publishable_entity in publishable_entities_in_list
 
 
     def test_next_version_with_different_different_title(self):

From fe9ccf9a2d53efe78c35cfe83caf14a61ab56d11 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 13 Feb 2025 10:58:20 -0800
Subject: [PATCH 02/27] test: Expand test cases

---
 .../apps/authoring/components/test_api.py     |  11 ++
 .../apps/authoring/units/test_api.py          | 119 +++++++++++++++++-
 2 files changed, 127 insertions(+), 3 deletions(-)

diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py
index 279a0335..70e5b582 100644
--- a/tests/openedx_learning/apps/authoring/components/test_api.py
+++ b/tests/openedx_learning/apps/authoring/components/test_api.py
@@ -43,6 +43,17 @@ def setUpTestData(cls) -> None:
         cls.problem_type = components_api.get_or_create_component_type("xblock.v1", "problem")
         cls.video_type = components_api.get_or_create_component_type("xblock.v1", "video")
 
+    def publish_component(self, component: Component):
+        """
+        Helper method to publish a single component.
+        """
+        publishing_api.publish_from_drafts(
+            self.learning_package.pk,
+            draft_qset=publishing_api.get_all_drafts(self.learning_package.pk).filter(
+                entity=component.publishable_entity,
+            ),
+        )
+
 
 class PerformanceTestCase(ComponentTestCase):
     """
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index a805f6b6..d8b1a0c5 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -8,7 +8,7 @@
 class UnitTestCase(ComponentTestCase):
 
     def setUp(self) -> None:
-        self.component_1, self.component_version_1 = authoring_api.create_component_and_version(
+        self.component_1, self.component_1_v1 = authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
             local_key="Query Counting",
@@ -16,7 +16,7 @@ def setUp(self) -> None:
             created=self.now,
             created_by=None,
         )
-        self.component_2, self.component_version_2 = authoring_api.create_component_and_version(
+        self.component_2, self.component_2_v2 = authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
             local_key="Query Counting (2)",
@@ -82,7 +82,7 @@ def test_create_next_unit_version_with_two_components(self):
                 self.component_2.publishable_entity.id,
             ],
             draft_version_pks=[None, None],
-            published_version_pks=[None, None],
+            published_version_pks=[None, None],  # FIXME: why do we specify this?
             created=self.now,
             created_by=None,
         )
@@ -93,6 +93,119 @@ def test_create_next_unit_version_with_two_components(self):
             authoring_api.UnitListEntry(component_version=self.component_2.versioning.draft, pinned=False),
         ]
 
+    def test_add_component_after_publish(self):
+        """
+        Adding a component to a published unit will create a new version and
+        show that the unit has unpublished changes.
+        """
+        unit, unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        assert unit.versioning.draft == unit_version
+        assert unit.versioning.published is None
+        assert unit.versioning.has_unpublished_changes == True
+        # Publish the empty unit:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        unit.refresh_from_db()  # Reloading the unit is necessary
+        assert unit.versioning.has_unpublished_changes == False
+
+        # Add a published component (unpinned):
+        assert self.component_1.versioning.has_unpublished_changes == False
+        unit_version_v2 = authoring_api.create_next_unit_version(
+            unit=unit,
+            title=unit_version.title,
+            publishable_entities_pks=[
+                self.component_1.publishable_entity.id,
+            ],
+            draft_version_pks=[None],
+            published_version_pks=[None],  # FIXME: why do we specify this?
+            created=self.now,
+            created_by=None,
+        )
+        # Now the unit should have unpublished changes:
+        unit.refresh_from_db()  # Reloading the unit is necessary
+        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.draft == unit_version_v2
+        assert unit.versioning.published == unit_version
+
+    def test_modify_component_after_publish(self):
+        """
+        Modifying a component in a published unit will NOT create a new version
+        nor show that the unit has unpublished changes. The modifications will
+        appear in the published version of the unit only after the component is
+        published.
+        """
+        # Create a unit:
+        unit, unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        # Add a draft component (unpinned):
+        assert self.component_1.versioning.has_unpublished_changes == True
+        unit_version_v2 = authoring_api.create_next_unit_version(
+            unit=unit,
+            title=unit_version.title,
+            publishable_entities_pks=[
+                self.component_1.publishable_entity.id,
+            ],
+            draft_version_pks=[None],
+            published_version_pks=[None],  # FIXME: why do we specify this?
+            created=self.now,
+            created_by=None,
+        )
+        # Publish the unit and the component:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        unit.refresh_from_db()  # Reloading the unit is necessary
+        self.component_1.refresh_from_db()
+        assert unit.versioning.has_unpublished_changes == False
+        assert self.component_1.versioning.has_unpublished_changes == False
+
+        # Now modify the component by changing its title (it remains a draft):
+        component_1_v2 = authoring_api.create_next_component_version(
+            self.component_1.pk,
+            content_to_replace={},
+            title="Modified Counting Problem with new title",
+            created=self.now,
+            created_by=None,
+        )
+
+        # The component now has unpublished changes, but the unit doesn't (⭐️ Is this what we want? ⭐️)
+        unit.refresh_from_db()  # Reloading the unit is necessary
+        self.component_1.refresh_from_db()
+        assert unit.versioning.has_unpublished_changes == False
+        assert self.component_1.versioning.has_unpublished_changes == True
+
+        # Since the component changes haven't been published, they should only appear in the draft unit
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+        ]
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=False),  # old version
+        ]
+
+        # But if we publish the component, the changes will appear in the published version of the unit.
+        self.publish_component(self.component_1)
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+        ]
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+        ]
+
+
+    # Test that only components can be added to units
+    # Test that components must be in the same learning package
+    # Test that _version_pks=[] arguments must be related to publishable_entities_pks
+    # Test that publishing a unit publishes its components
+    # Test viewing old snapshots of units and components by passing in a timestamp to some get_historic_unit() API
+
 
     def test_next_version_with_different_different_title(self):
         """Test creating a unit version with a different title.

From 7eaa20635838ee7f53cd5a869c825a80a3001b8a Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 10:48:18 -0800
Subject: [PATCH 03/27] feat: Add a contains_unpublished_changes API

---
 .../apps/authoring/containers/api.py          | 43 +++++++++++++++++++
 .../apps/authoring/units/test_api.py          | 33 +++++++++-----
 2 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index adaa01f6..d67126cd 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -34,6 +34,7 @@
     "ContainerEntityListEntry",
     "get_entities_in_draft_container",
     "get_entities_in_published_container",
+    "contains_unpublished_changes",
 ]
 
 
@@ -538,3 +539,45 @@ def get_entities_in_published_container(
             pinned=row.published_version is not None,
         ))
     return entity_list
+
+
+def contains_unpublished_changes(
+    container: ContainerEntity | ContainerEntityMixin,
+) -> bool:
+    """
+    Check recursively if a container has any unpublished changes.
+
+    Note: container.versioning.has_unpublished_changes only checks if the container
+    itself has unpublished changes, not if its contents do.
+    """
+    if isinstance(container, ContainerEntityMixin):
+        container = container.container_entity
+    assert isinstance(container, ContainerEntity)
+
+    if container.versioning.has_unpublished_changes:
+        return True
+
+    # We only care about children that are un-pinned, since published changes to pinned children don't matter
+    defined_list = container.versioning.draft.defined_list
+
+    # TODO: This is a naive inefficient implementation but hopefully correct.
+    # Once we know it's correct and have a good test suite, then we can optimize.
+    # We will likely change to a tracking-based approach rather than a "scan for changes" based approach.
+    for row in defined_list.entitylistrow_set.filter(draft_version=None):
+        try:
+            child_container = row.entity.containerentity
+        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:
+            child_container = None
+        if child_container:
+            child_container = row.entity.containerentity
+            # This is itself a container - check recursively:
+            if child_container.versioning.has_unpublished_changes or contains_unpublished_changes(child_container):
+                return True
+        else:
+            # This is not a container:
+            draft_pk = row.entity.draft.version_id if row.entity.draft else None
+            published_pk = row.entity.published.version_id if row.entity.published else None
+            if draft_pk != published_pk:
+                return True
+    
+    return False
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index d8b1a0c5..22fa1807 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -111,7 +111,8 @@ def test_add_component_after_publish(self):
         # Publish the empty unit:
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()  # Reloading the unit is necessary
-        assert unit.versioning.has_unpublished_changes == False
+        assert unit.versioning.has_unpublished_changes == False  # Shallow check for just the unit itself, not children
+        assert authoring_api.contains_unpublished_changes(unit) == False  # Deeper check
 
         # Add a published component (unpinned):
         assert self.component_1.versioning.has_unpublished_changes == False
@@ -128,16 +129,17 @@ def test_add_component_after_publish(self):
         )
         # Now the unit should have unpublished changes:
         unit.refresh_from_db()  # Reloading the unit is necessary
-        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes == True  # Shallow check - adding a child is a change to the unit
+        assert authoring_api.contains_unpublished_changes(unit) == True  # Deeper check
         assert unit.versioning.draft == unit_version_v2
         assert unit.versioning.published == unit_version
 
     def test_modify_component_after_publish(self):
         """
         Modifying a component in a published unit will NOT create a new version
-        nor show that the unit has unpublished changes. The modifications will
-        appear in the published version of the unit only after the component is
-        published.
+        nor show that the unit has unpublished changes (but it will "contain"
+        unpublished changes). The modifications will appear in the published
+        version of the unit only after the component is published.
         """
         # Create a unit:
         unit, unit_version = authoring_api.create_unit_and_version(
@@ -164,7 +166,8 @@ def test_modify_component_after_publish(self):
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()  # Reloading the unit is necessary
         self.component_1.refresh_from_db()
-        assert unit.versioning.has_unpublished_changes == False
+        assert unit.versioning.has_unpublished_changes == False  # Shallow check
+        assert authoring_api.contains_unpublished_changes(unit) == False  # Deeper check
         assert self.component_1.versioning.has_unpublished_changes == False
 
         # Now modify the component by changing its title (it remains a draft):
@@ -176,10 +179,11 @@ def test_modify_component_after_publish(self):
             created_by=None,
         )
 
-        # The component now has unpublished changes, but the unit doesn't (⭐️ Is this what we want? ⭐️)
+        # The component now has unpublished changes; the unit doesn't directly but does contain
         unit.refresh_from_db()  # Reloading the unit is necessary
         self.component_1.refresh_from_db()
-        assert unit.versioning.has_unpublished_changes == False
+        assert unit.versioning.has_unpublished_changes == False  # Shallow check should be false - unit is unchanged
+        assert authoring_api.contains_unpublished_changes(unit) == True  # But unit DOES contain changes
         assert self.component_1.versioning.has_unpublished_changes == True
 
         # Since the component changes haven't been published, they should only appear in the draft unit
@@ -198,13 +202,22 @@ def test_modify_component_after_publish(self):
         assert authoring_api.get_components_in_published_unit(unit) == [
             authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
         ]
+        assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
 
 
+    # Test query count of contains_unpublished_changes()
     # Test that only components can be added to units
     # Test that components must be in the same learning package
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks
-    # Test that publishing a unit publishes its components
-    # Test viewing old snapshots of units and components by passing in a timestamp to some get_historic_unit() API
+    # Test that publishing a unit publishes its child components (either automatically or throws an exception if you
+    #     don't request the children be published together with the containeer)
+    # Test that publishing a component does NOT publish changes to its parent unit
+    # Test that I can get a history of a given unit and all its children, including children that aren't currently in
+    #     the unit and excluding children that are only in other units.
+    # Test that I can get a history of a given unit and its children, that includes changes made to the child components
+    #     while they were part of the unit but excludes changes made to those children while they were not part of
+    #     the unit. 🫣
+    # Test viewing old snapshots of units and components by passing in a timestamp to some get_historic_unit() API?
 
 
     def test_next_version_with_different_different_title(self):

From 3ac1e1169965d92e33ad7cd407c8288550941cb6 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 11:07:40 -0800
Subject: [PATCH 04/27] perf: Optimize contains_unpublished_changes to check a
 unit using only 3 queries

---
 .../apps/authoring/containers/api.py          | 17 +++++++-
 .../apps/authoring/units/test_api.py          | 39 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index d67126cd..cc9b60ff 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -551,7 +551,16 @@ def contains_unpublished_changes(
     itself has unpublished changes, not if its contents do.
     """
     if isinstance(container, ContainerEntityMixin):
-        container = container.container_entity
+        # The query below pre-loads the data we need but is otherwise the same thing as:
+        #     container = container.container_entity
+        container = ContainerEntity.objects.select_related(
+            "publishable_entity",
+            "publishable_entity__draft",
+            "publishable_entity__draft__version",
+            "publishable_entity__draft__version__containerentityversion__defined_list",
+        ).get(pk=container.container_entity)
+    else:
+        pass # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit?
     assert isinstance(container, ContainerEntity)
 
     if container.versioning.has_unpublished_changes:
@@ -563,7 +572,11 @@ def contains_unpublished_changes(
     # TODO: This is a naive inefficient implementation but hopefully correct.
     # Once we know it's correct and have a good test suite, then we can optimize.
     # We will likely change to a tracking-based approach rather than a "scan for changes" based approach.
-    for row in defined_list.entitylistrow_set.filter(draft_version=None):
+    for row in defined_list.entitylistrow_set.filter(draft_version=None).select_related(
+        "entity__containerentity",
+        "entity__draft__version",
+        "entity__published__version",
+    ):
         try:
             child_container = row.entity.containerentity
         except PublishableEntity.containerentity.RelatedObjectDoesNotExist:
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 22fa1807..abcfe5ed 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -205,7 +205,44 @@ def test_modify_component_after_publish(self):
         assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
 
 
-    # Test query count of contains_unpublished_changes()
+    def test_query_count_of_contains_unpublished_changes(self):
+        """
+        Checking for unpublished changes in a unit should require a fixed number
+        of queries, not get more expensive as the unit gets larger.
+        """
+        unit, unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        # Add 100 components (unpinned)
+        component_count = 100
+        publishable_entities_pks = []
+        for i in range(0, component_count):
+            component, _version = authoring_api.create_component_and_version(
+                self.learning_package.id,
+                component_type=self.problem_type,
+                local_key=f"Query Counting {i}",
+                title=f"Querying Counting Problem {i}",
+                created=self.now,
+            )
+            publishable_entities_pks.append(component.publishable_entity_id)
+        authoring_api.create_next_unit_version(
+            unit=unit,
+            title=unit_version.title,
+            publishable_entities_pks=publishable_entities_pks,
+            draft_version_pks=[None] * component_count,
+            published_version_pks=[None] * component_count,  # FIXME: why do we specify this?
+            created=self.now,
+        )
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        unit.refresh_from_db()
+        with self.assertNumQueries(3):
+            assert authoring_api.contains_unpublished_changes(unit) == False
+
+    # Test that pinned components with changes don't show up as "contains unpublished changes"
     # Test that only components can be added to units
     # Test that components must be in the same learning package
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks

From 236f7cc92ff0cd33e8cf8f4abe6235eb917cb784 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 15:13:28 -0800
Subject: [PATCH 05/27] refactor: Simplify EntityList: merge
 draft_version+published_version -> entity_version

---
 .../apps/authoring/containers/api.py          | 63 ++++++++-----------
 .../containers/migrations/0001_initial.py     |  5 +-
 .../apps/authoring/containers/models.py       | 10 +--
 openedx_learning/apps/authoring/units/api.py  | 21 +++----
 .../apps/authoring/units/test_api.py          | 12 ++--
 5 files changed, 43 insertions(+), 68 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index cc9b60ff..246773ce 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -81,8 +81,7 @@ def create_next_defined_list(
     previous_entity_list: EntityList | None,
     new_entity_list: EntityList,
     entity_pks: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
 ) -> EntityListRow:
     """
     Create new entity list rows for an entity list.
@@ -91,8 +90,9 @@ def create_next_defined_list(
         previous_entity_list: The previous entity list that the new entity list is based on.
         new_entity_list: The entity list to create the rows for.
         entity_pks: The IDs of the publishable entities that the entity list rows reference.
-        draft_version_pks: The IDs of the draft versions of the entities (PublishableEntityVersion) that the entity list rows reference.
-        published_version_pks: The IDs of the published versions of the entities (PublishableEntityVersion) that the entity list rows reference.
+        entity_version_pks: The IDs of the draft versions of the entities
+            (PublishableEntityVersion) that the entity list rows reference, or
+            Nones for "unpinned" (default).
 
     Returns:
         The newly created entity list rows.
@@ -109,8 +109,8 @@ def create_next_defined_list(
         current_rows = previous_entity_list.entitylistrow_set.all()
         publishable_entities_in_rows = {row.entity.pk: row for row in current_rows}
         new_rows = []
-        for order_num, entity_pk, draft_version_pk, published_version_pk in zip(
-            order_nums, entity_pks, draft_version_pks, published_version_pks
+        for order_num, entity_pk, entity_version_pk in zip(
+            order_nums, entity_pks, entity_version_pks
         ):
             row = publishable_entities_in_rows.get(entity_pk)
             if row and row.order_num == order_num:
@@ -121,8 +121,7 @@ def create_next_defined_list(
                     entity_list=new_entity_list,
                     entity_id=entity_pk,
                     order_num=order_num,
-                    draft_version_id=draft_version_pk,
-                    published_version_id=published_version_pk,
+                    entity_version_id=entity_version_pk,
                 )
             )
         EntityListRow.objects.bulk_create(new_rows)
@@ -130,8 +129,7 @@ def create_next_defined_list(
 
 def create_defined_list_with_rows(
     entity_pks: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
 ) -> EntityList:
     """
     Create new entity list rows for an entity list.
@@ -139,8 +137,9 @@ def create_defined_list_with_rows(
     Args:
         entity_list: The entity list to create the rows for.
         entity_pks: The IDs of the publishable entities that the entity list rows reference.
-        draft_version_pks: The IDs of the draft versions of the entities (PublishableEntityVersion) that the entity list rows reference.
-        published_version_pks: The IDs of the published versions of the entities (PublishableEntityVersion) that the entity list rows reference.
+        entity_version_pks: The IDs of the versions of the entities
+            (PublishableEntityVersion) that the entity list rows reference, or
+            Nones for "unpinned" (default).
 
     Returns:
         The newly created entity list.
@@ -154,11 +153,10 @@ def create_defined_list_with_rows(
                     entity_list=entity_list,
                     entity_id=entity_pk,
                     order_num=order_num,
-                    draft_version_id=draft_version_pk,
-                    published_version_id=published_version_pk,
+                    entity_version_id=entity_version_pk,
                 )
-                for order_num, entity_pk, draft_version_pk, published_version_pk in zip(
-                    order_nums, entity_pks, draft_version_pks, published_version_pks
+                for order_num, entity_pk, entity_version_pk in zip(
+                    order_nums, entity_pks, entity_version_pks
                 )
             ]
         )
@@ -186,8 +184,7 @@ def get_entity_list_with_pinned_versions(
                     entity_list=entity_list,
                     entity_id=row.entity.id,
                     order_num=row.order_num,
-                    draft_version_id=None,
-                    published_version_id=None, # For simplicity, we are not copying the pinned versions
+                    entity_version_id=None,  # For simplicity, we are not copying the pinned versions
                 )
                 for row in rows
             ]
@@ -210,7 +207,7 @@ def check_unpinned_versions_in_defined_list(
     """
     # Is there a way to short-circuit this?
     return any(
-        row.draft_version is None or row.published_version is None
+        row.entity_version is None
         for row in defined_list.entitylistrow_set.all()
     )
 
@@ -239,8 +236,7 @@ def create_container_version(
     version_num: int,
     title: str,
     publishable_entities_pk: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
     entity: PublishableEntity,
     created: datetime,
     created_by: int | None,
@@ -270,8 +266,7 @@ def create_container_version(
         )
         defined_list = create_defined_list_with_rows(
             entity_pks=publishable_entities_pk,
-            draft_version_pks=draft_version_pks,
-            published_version_pks=published_version_pks,
+            entity_version_pks=entity_version_pks,
         )
         container_version = ContainerEntityVersion.objects.create(
             publishable_entity_version=publishable_entity_version,
@@ -290,8 +285,7 @@ def create_next_container_version(
     container_pk: int,
     title: str,
     publishable_entities_pk: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
     entity: PublishableEntity,
     created: datetime,
     created_by: int | None,
@@ -349,8 +343,7 @@ def create_next_container_version(
                 previous_entity_list=last_version.defined_list,
                 new_entity_list=create_entity_list(),
                 entity_pks=publishable_entities_pk,
-                draft_version_pks=draft_version_pks,
-                published_version_pks=published_version_pks,
+                entity_version_pks=entity_version_pks,
             )
             next_initial_list = get_entity_list_with_pinned_versions(
                 rows=next_defined_list.entitylistrow_set.all()
@@ -385,8 +378,7 @@ def create_container_and_version(
     created_by: int | None,
     title: str,
     publishable_entities_pk: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
 ) -> ContainerEntityVersion:
     """
     Create a new container and its first version.
@@ -410,8 +402,7 @@ def create_container_and_version(
             version_num=1,
             title=title,
             publishable_entities_pk=publishable_entities_pk,
-            draft_version_pks=draft_version_pks,
-            published_version_pks=published_version_pks,
+            entity_version_pks=entity_version_pks,
             entity=container.publishable_entity,
             created=created,
             created_by=created_by,
@@ -507,8 +498,8 @@ def get_entities_in_draft_container(
     defined_list = container.versioning.draft.defined_list
     for row in defined_list.entitylistrow_set.order_by("order_num"):
         entity_list.append(ContainerEntityListEntry(
-            entity_version=row.draft_version or row.entity.draft.version,
-            pinned=row.draft_version is not None,
+            entity_version=row.entity_version or row.entity.draft.version,
+            pinned=row.entity_version is not None,
         ))
     return entity_list
 
@@ -535,8 +526,8 @@ def get_entities_in_published_container(
     entity_list = []
     for row in cev.defined_list.entitylistrow_set.order_by("order_num"):
         entity_list.append(ContainerEntityListEntry(
-            entity_version=row.published_version or row.entity.published.version,
-            pinned=row.published_version is not None,
+            entity_version=row.entity_version or row.entity.published.version,
+            pinned=row.entity_version is not None,
         ))
     return entity_list
 
@@ -572,7 +563,7 @@ def contains_unpublished_changes(
     # TODO: This is a naive inefficient implementation but hopefully correct.
     # Once we know it's correct and have a good test suite, then we can optimize.
     # We will likely change to a tracking-based approach rather than a "scan for changes" based approach.
-    for row in defined_list.entitylistrow_set.filter(draft_version=None).select_related(
+    for row in defined_list.entitylistrow_set.filter(entity_version=None).select_related(
         "entity__containerentity",
         "entity__draft__version",
         "entity__published__version",
diff --git a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
index 47c4a4ab..26583c98 100644
--- a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
+++ b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
@@ -1,4 +1,4 @@
-# Generated by Django 4.2.16 on 2024-10-29 12:57
+# Generated by Django 4.2.19 on 2025-02-14 23:04
 
 from django.db import migrations, models
 import django.db.models.deletion
@@ -33,10 +33,9 @@ class Migration(migrations.Migration):
             fields=[
                 ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                 ('order_num', models.PositiveIntegerField()),
-                ('draft_version', models.ForeignKey(null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='draft_version', to='oel_publishing.publishableentityversion')),
                 ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')),
                 ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.entitylist')),
-                ('published_version', models.ForeignKey(null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='published_version', to='oel_publishing.publishableentityversion')),
+                ('entity_version', models.ForeignKey(null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')),
             ],
         ),
         migrations.CreateModel(
diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py
index 66588bf3..ce478a0a 100644
--- a/openedx_learning/apps/authoring/containers/models.py
+++ b/openedx_learning/apps/authoring/containers/models.py
@@ -58,17 +58,11 @@ class EntityListRow(models.Model):
     # So our approach to this is to use a value of None (null) to represent an
     # unpinned reference to a PublishableEntity. It's shorthand for "just use
     # the latest draft or published version of this, as appropriate".
-    draft_version = models.ForeignKey(
+    entity_version = models.ForeignKey(
         PublishableEntityVersion,
         on_delete=models.RESTRICT,
         null=True,
-        related_name="draft_version",
-    )
-    published_version = models.ForeignKey(
-        PublishableEntityVersion,
-        on_delete=models.RESTRICT,
-        null=True,
-        related_name="published_version",
+        related_name="+",  # Do we need the reverse relation?
     )
 
 
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index a42324fc..ae1d8ab3 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -60,8 +60,7 @@ def create_unit_version(
     version_num: int,
     title: str,
     publishable_entities_pks: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
     created: datetime,
     created_by: int | None = None,
 ) -> Unit:
@@ -82,8 +81,7 @@ def create_unit_version(
             version_num,
             title,
             publishable_entities_pks,
-            draft_version_pks,
-            published_version_pks,
+            entity_version_pks,
             unit.container_entity.publishable_entity,
             created,
             created_by,
@@ -100,8 +98,7 @@ def create_next_unit_version(
     unit: Unit,
     title: str,
     publishable_entities_pks: list[int],
-    draft_version_pks: list[int | None],
-    published_version_pks: list[int | None],
+    entity_version_pks: list[int | None],
     created: datetime,
     created_by: int | None = None,
 ) -> Unit:
@@ -122,8 +119,7 @@ def create_next_unit_version(
             unit.container_entity.pk,
             title,
             publishable_entities_pks,
-            draft_version_pks,
-            published_version_pks,
+            entity_version_pks,
             unit.container_entity.publishable_entity,
             created,
             created_by,
@@ -157,11 +153,10 @@ def create_unit_and_version(
             unit,
             1,
             title,
-            [],
-            [],
-            [],
-            created,
-            created_by,
+            publishable_entities_pks=[],
+            entity_version_pks=[],
+            created=created,
+            created_by=created_by,
         )
     return unit, unit_version
 
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index abcfe5ed..472bfd54 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -81,8 +81,7 @@ def test_create_next_unit_version_with_two_components(self):
                 self.component_1.publishable_entity.id,
                 self.component_2.publishable_entity.id,
             ],
-            draft_version_pks=[None, None],
-            published_version_pks=[None, None],  # FIXME: why do we specify this?
+            entity_version_pks=[None, None],
             created=self.now,
             created_by=None,
         )
@@ -122,8 +121,7 @@ def test_add_component_after_publish(self):
             publishable_entities_pks=[
                 self.component_1.publishable_entity.id,
             ],
-            draft_version_pks=[None],
-            published_version_pks=[None],  # FIXME: why do we specify this?
+            entity_version_pks=[None],
             created=self.now,
             created_by=None,
         )
@@ -157,8 +155,7 @@ def test_modify_component_after_publish(self):
             publishable_entities_pks=[
                 self.component_1.publishable_entity.id,
             ],
-            draft_version_pks=[None],
-            published_version_pks=[None],  # FIXME: why do we specify this?
+            entity_version_pks=[None],
             created=self.now,
             created_by=None,
         )
@@ -233,8 +230,7 @@ def test_query_count_of_contains_unpublished_changes(self):
             unit=unit,
             title=unit_version.title,
             publishable_entities_pks=publishable_entities_pks,
-            draft_version_pks=[None] * component_count,
-            published_version_pks=[None] * component_count,  # FIXME: why do we specify this?
+            entity_version_pks=[None] * component_count,
             created=self.now,
         )
         authoring_api.publish_all_drafts(self.learning_package.id)

From 5f0823165468dfdae0c6f3eb00d0fcc65cc884cb Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 15:37:32 -0800
Subject: [PATCH 06/27] refactor: Simplify create_next_unit_version() API

---
 openedx_learning/apps/authoring/units/api.py  | 17 ++++++++---
 .../apps/authoring/units/test_api.py          | 29 +++++--------------
 2 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index ae1d8ab3..5c854e98 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -6,7 +6,7 @@
 
 from django.db.transaction import atomic
 
-from openedx_learning.apps.authoring.components.models import ComponentVersion
+from openedx_learning.apps.authoring.components.models import Component, ComponentVersion
 from openedx_learning.apps.authoring.containers.models import EntityListRow
 from ..publishing import api as publishing_api
 from ..containers import api as container_api
@@ -97,8 +97,7 @@ def create_unit_version(
 def create_next_unit_version(
     unit: Unit,
     title: str,
-    publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None],
+    components: list[Component | ComponentVersion],
     created: datetime,
     created_by: int | None = None,
 ) -> Unit:
@@ -107,11 +106,21 @@ def create_next_unit_version(
     Args:
         unit_pk: The unit ID.
         title: The title.
-        publishable_entities_pk: The components.
+        components: The components, as a list of Components (unpinned) and/or ComponentVersions (pinned)
         entity: The entity.
         created: The creation date.
         created_by: The user who created the unit.
     """
+    for c in components:
+        assert isinstance(c, (Component, ComponentVersion))
+    publishable_entities_pks = [
+        (c.publishable_entity_id if isinstance(c, Component) else c.component.publishable_entity_id)
+        for c in components
+    ]
+    entity_version_pks = [
+        (cv.pk if isinstance(cv, ComponentVersion) else None)
+        for cv in components
+    ]
     with atomic():
         # TODO: how can we enforce that publishable entities must be components?
         # This currently allows for any publishable entity to be added to a unit.
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 472bfd54..e7d2e057 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -57,15 +57,13 @@ def test_create_empty_unit_and_version(self):
         assert unit.versioning.published is None
 
     def test_create_next_unit_version_with_two_components(self):
-        """Test creating a unit version with two components.
+        """Test creating a unit version with two unpinned components.
 
         Expected results:
         1. A new unit version is created.
         2. The unit version number is 2.
         3. The unit version is in the unit's versions.
-        4. The components are in the unit version's user defined list.
-        5. Initial list contains the pinned versions of the defined list.
-        6. Frozen list is empty.
+        4. The components are in the draft unit version's component list and are unpinned.
         """
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
@@ -77,11 +75,7 @@ def test_create_next_unit_version_with_two_components(self):
         unit_version_v2 = authoring_api.create_next_unit_version(
             unit=unit,
             title="Unit",
-            publishable_entities_pks=[
-                self.component_1.publishable_entity.id,
-                self.component_2.publishable_entity.id,
-            ],
-            entity_version_pks=[None, None],
+            components=[self.component_1, self.component_2],
             created=self.now,
             created_by=None,
         )
@@ -118,10 +112,7 @@ def test_add_component_after_publish(self):
         unit_version_v2 = authoring_api.create_next_unit_version(
             unit=unit,
             title=unit_version.title,
-            publishable_entities_pks=[
-                self.component_1.publishable_entity.id,
-            ],
-            entity_version_pks=[None],
+            components=[self.component_1],
             created=self.now,
             created_by=None,
         )
@@ -152,10 +143,7 @@ def test_modify_component_after_publish(self):
         unit_version_v2 = authoring_api.create_next_unit_version(
             unit=unit,
             title=unit_version.title,
-            publishable_entities_pks=[
-                self.component_1.publishable_entity.id,
-            ],
-            entity_version_pks=[None],
+            components=[self.component_1],
             created=self.now,
             created_by=None,
         )
@@ -216,7 +204,7 @@ def test_query_count_of_contains_unpublished_changes(self):
         )
         # Add 100 components (unpinned)
         component_count = 100
-        publishable_entities_pks = []
+        components = []
         for i in range(0, component_count):
             component, _version = authoring_api.create_component_and_version(
                 self.learning_package.id,
@@ -225,12 +213,11 @@ def test_query_count_of_contains_unpublished_changes(self):
                 title=f"Querying Counting Problem {i}",
                 created=self.now,
             )
-            publishable_entities_pks.append(component.publishable_entity_id)
+            components.append(component)
         authoring_api.create_next_unit_version(
             unit=unit,
             title=unit_version.title,
-            publishable_entities_pks=publishable_entities_pks,
-            entity_version_pks=[None] * component_count,
+            components=components,
             created=self.now,
         )
         authoring_api.publish_all_drafts(self.learning_package.id)

From 3545527a4d4c38ec58e9ae4e5cb1141c61a4caa7 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 16:21:20 -0800
Subject: [PATCH 07/27] test: add tests for pinned components

---
 openedx_learning/api/authoring_models.py      |   2 +
 .../apps/authoring/containers/models.py       |   5 +
 openedx_learning/apps/authoring/units/api.py  |   5 +-
 .../apps/authoring/units/models.py            |   5 +
 .../apps/authoring/units/test_api.py          | 171 +++++++++++++-----
 5 files changed, 141 insertions(+), 47 deletions(-)

diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py
index fb0ab669..f8653b2d 100644
--- a/openedx_learning/api/authoring_models.py
+++ b/openedx_learning/api/authoring_models.py
@@ -12,3 +12,5 @@
 from ..apps.authoring.contents.models import *
 from ..apps.authoring.publishing.model_mixins import *
 from ..apps.authoring.publishing.models import *
+from ..apps.authoring.containers.models import *
+from ..apps.authoring.units.models import *
diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py
index ce478a0a..be1ac0b6 100644
--- a/openedx_learning/apps/authoring/containers/models.py
+++ b/openedx_learning/apps/authoring/containers/models.py
@@ -9,6 +9,11 @@
     PublishableEntityVersionMixin,
 )
 
+__all__ = [
+    "ContainerEntity",
+    "ContainerEntityVersion",
+]
+
 
 class EntityList(models.Model):
     """
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 5c854e98..597cc5d1 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -265,8 +265,11 @@ def get_components_in_published_unit(
     given container.
     """
     assert isinstance(unit, (Unit, UnitVersion))
+    published_entities = container_api.get_entities_in_published_container(unit)
+    if published_entities == None:
+        return None  # There is no published version of this unit. Should this be an exception?
     entity_list = []
-    for entry in container_api.get_entities_in_published_container(unit):
+    for entry in published_entities:
         # Convert from generic PublishableEntityVersion to ComponentVersion:
         component_version = entry.entity_version.componentversion
         assert isinstance(component_version, ComponentVersion)
diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py
index a9d167a2..502b34a6 100644
--- a/openedx_learning/apps/authoring/units/models.py
+++ b/openedx_learning/apps/authoring/units/models.py
@@ -2,6 +2,11 @@
 
 from ..containers.models_mixin import ContainerEntityMixin, ContainerEntityVersionMixin
 
+__all__ = [
+    "Unit",
+    "UnitVersion",
+]
+
 
 class Unit(ContainerEntityMixin):
     """
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index e7d2e057..f67925dd 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -3,6 +3,7 @@
 """
 from ..components.test_api import ComponentTestCase
 from openedx_learning.api import authoring as authoring_api
+from openedx_learning.api import authoring_models
 
 
 class UnitTestCase(ComponentTestCase):
@@ -16,7 +17,7 @@ def setUp(self) -> None:
             created=self.now,
             created_by=None,
         )
-        self.component_2, self.component_2_v2 = authoring_api.create_component_and_version(
+        self.component_2, self.component_2_v1 = authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
             local_key="Query Counting (2)",
@@ -25,6 +26,49 @@ def setUp(self) -> None:
             created_by=None,
         )
 
+    def create_unit_with_components(
+        self,
+        components: list[authoring_models.Component | authoring_models.ComponentVersion],
+        *,
+        title="Unit",
+        key="unit:key",
+    ) -> authoring_models.Unit:
+        """ Helper method to quickly create a unit with some components """
+        unit, _unit_v1 = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=key,
+            title=title,
+            created=self.now,
+            created_by=None,
+        )
+        _unit_v2 = authoring_api.create_next_unit_version(
+            unit=unit,
+            title=title,
+            components=components,
+            created=self.now,
+            created_by=None,
+        )
+        unit.refresh_from_db()
+        return unit
+    
+    def modify_component(
+        self,
+        component: authoring_models.Component,
+        *,
+        title="Modified Component",
+        timestamp=None,
+    ) -> authoring_models.ComponentVersion:
+        """
+        Helper method to modify a component for the purposes of testing units/drafts/pinning/publishing/etc.
+        """
+        return authoring_api.create_next_component_version(
+            component.pk,
+            content_to_replace={},
+            title=title,
+            created=timestamp or self.now,
+            created_by=None,
+        )
+
     def test_create_unit_with_content_instead_of_components(self):
         """Test creating a unit with content instead of components.
 
@@ -56,7 +100,7 @@ def test_create_empty_unit_and_version(self):
         assert unit.versioning.draft == unit_version
         assert unit.versioning.published is None
 
-    def test_create_next_unit_version_with_two_components(self):
+    def test_create_next_unit_version_with_two_unpinned_components(self):
         """Test creating a unit version with two unpinned components.
 
         Expected results:
@@ -85,6 +129,33 @@ def test_create_next_unit_version_with_two_components(self):
             authoring_api.UnitListEntry(component_version=self.component_1.versioning.draft, pinned=False),
             authoring_api.UnitListEntry(component_version=self.component_2.versioning.draft, pinned=False),
         ]
+        assert authoring_api.get_components_in_published_unit(unit) is None
+
+    def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
+        """
+        Test creating a unit version with one unpinned and one pinned component.
+        """
+        unit, unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        unit_version_v2 = authoring_api.create_next_unit_version(
+            unit=unit,
+            title="Unit",
+            components=[self.component_1, self.component_2_v1],  # Note the "v1" pinning it to version 2
+            created=self.now,
+            created_by=None,
+        )
+        assert unit_version_v2.version_num == 2
+        assert unit_version_v2 in unit.versioning.versions.all()
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=False),
+            authoring_api.UnitListEntry(component_version=self.component_2_v1, pinned=True),  # Pinned to v1
+        ]
+        assert authoring_api.get_components_in_published_unit(unit) is None
 
     def test_add_component_after_publish(self):
         """
@@ -123,49 +194,31 @@ def test_add_component_after_publish(self):
         assert unit.versioning.draft == unit_version_v2
         assert unit.versioning.published == unit_version
 
-    def test_modify_component_after_publish(self):
+    def test_modify_unpinned_component_after_publish(self):
         """
-        Modifying a component in a published unit will NOT create a new version
-        nor show that the unit has unpublished changes (but it will "contain"
-        unpublished changes). The modifications will appear in the published
-        version of the unit only after the component is published.
+        Modifying an unpinned component in a published unit will NOT create a
+        new version nor show that the unit has unpublished changes (but it will
+        "contain" unpublished changes). The modifications will appear in the
+        published version of the unit only after the component is published.
         """
-        # Create a unit:
-        unit, unit_version = authoring_api.create_unit_and_version(
-            learning_package_id=self.learning_package.id,
-            key=f"unit:key",
-            title="Unit",
-            created=self.now,
-            created_by=None,
-        )
-        # Add a draft component (unpinned):
+        # Create a unit with one unpinned draft component:
         assert self.component_1.versioning.has_unpublished_changes == True
-        unit_version_v2 = authoring_api.create_next_unit_version(
-            unit=unit,
-            title=unit_version.title,
-            components=[self.component_1],
-            created=self.now,
-            created_by=None,
-        )
+        unit = self.create_unit_with_components([self.component_1])
+        assert unit.versioning.has_unpublished_changes == True
+
         # Publish the unit and the component:
         authoring_api.publish_all_drafts(self.learning_package.id)
-        unit.refresh_from_db()  # Reloading the unit is necessary
+        unit.refresh_from_db()  # Reloading the unit is necessary if we accessed 'versioning' before publish
         self.component_1.refresh_from_db()
         assert unit.versioning.has_unpublished_changes == False  # Shallow check
         assert authoring_api.contains_unpublished_changes(unit) == False  # Deeper check
         assert self.component_1.versioning.has_unpublished_changes == False
 
         # Now modify the component by changing its title (it remains a draft):
-        component_1_v2 = authoring_api.create_next_component_version(
-            self.component_1.pk,
-            content_to_replace={},
-            title="Modified Counting Problem with new title",
-            created=self.now,
-            created_by=None,
-        )
+        component_1_v2 = self.modify_component(self.component_1, title="Modified Counting Problem with new title")
 
         # The component now has unpublished changes; the unit doesn't directly but does contain
-        unit.refresh_from_db()  # Reloading the unit is necessary
+        unit.refresh_from_db()  # Reloading the unit is necessary, or 'unit.versioning' will be outdated
         self.component_1.refresh_from_db()
         assert unit.versioning.has_unpublished_changes == False  # Shallow check should be false - unit is unchanged
         assert authoring_api.contains_unpublished_changes(unit) == True  # But unit DOES contain changes
@@ -189,19 +242,46 @@ def test_modify_component_after_publish(self):
         ]
         assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
 
+    def test_modify_pinned_component(self):
+        """
+        When a pinned 📌 component in unit is modified and/or published, it will
+        have no effect on either the draft nor published version of the unit,
+        which will continue to use the pinned version.
+        """
+        # Create a unit with one component (pinned 📌 to v1):
+        unit = self.create_unit_with_components([self.component_1_v1])
+
+        # Publish the unit and the component:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        expected_unit_contents = [
+            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=True),  # pinned 📌 to v1
+        ]
+        assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
+
+        # Now modify the component by changing its title (it remains a draft):
+        self.modify_component(self.component_1, title="Modified Counting Problem with new title")
+
+        # The component now has unpublished changes; the unit is entirely unaffected
+        unit.refresh_from_db()  # Reloading the unit is necessary, or 'unit.versioning' will be outdated
+        self.component_1.refresh_from_db()
+        assert unit.versioning.has_unpublished_changes == False  # Shallow check
+        assert authoring_api.contains_unpublished_changes(unit) == False  # Deep check
+        assert self.component_1.versioning.has_unpublished_changes == True
+
+        # Neither the draft nor the published version of the unit is affected
+        assert authoring_api.get_components_in_draft_unit(unit) == expected_unit_contents
+        assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
+        # Even if we publish the component, the unit stays pinned to the specified version:
+        self.publish_component(self.component_1)
+        assert authoring_api.get_components_in_draft_unit(unit) == expected_unit_contents
+        assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
+
 
     def test_query_count_of_contains_unpublished_changes(self):
         """
         Checking for unpublished changes in a unit should require a fixed number
         of queries, not get more expensive as the unit gets larger.
         """
-        unit, unit_version = authoring_api.create_unit_and_version(
-            learning_package_id=self.learning_package.id,
-            key=f"unit:key",
-            title="Unit",
-            created=self.now,
-            created_by=None,
-        )
         # Add 100 components (unpinned)
         component_count = 100
         components = []
@@ -214,18 +294,17 @@ def test_query_count_of_contains_unpublished_changes(self):
                 created=self.now,
             )
             components.append(component)
-        authoring_api.create_next_unit_version(
-            unit=unit,
-            title=unit_version.title,
-            components=components,
-            created=self.now,
-        )
+        unit = self.create_unit_with_components(components)
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()
         with self.assertNumQueries(3):
             assert authoring_api.contains_unpublished_changes(unit) == False
 
-    # Test that pinned components with changes don't show up as "contains unpublished changes"
+        # Modify the most recently created component:
+        self.modify_component(component, title="Modified Component")
+        with self.assertNumQueries(2):
+            assert authoring_api.contains_unpublished_changes(unit) == True
+
     # Test that only components can be added to units
     # Test that components must be in the same learning package
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks

From 275d5d0a7c3608f68ee233dc7374094231c962cc Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 16:46:16 -0800
Subject: [PATCH 08/27] feat: remove defined_list/initial_list/frozen_list from
 the API for now

---
 .../apps/authoring/containers/api.py          | 50 -------------------
 openedx_learning/apps/authoring/units/api.py  | 33 ------------
 2 files changed, 83 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 246773ce..a186beb7 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -28,9 +28,6 @@
     "create_next_container_version",
     "create_container_and_version",
     "get_container",
-    "get_defined_list_rows_for_container_version",
-    "get_initial_list_rows_for_container_version",
-    "get_frozen_list_rows_for_container_version",
     "ContainerEntityListEntry",
     "get_entities_in_draft_container",
     "get_entities_in_published_container",
@@ -424,53 +421,6 @@ def get_container(pk: int) -> ContainerEntity:
     return ContainerEntity.objects.get(pk=pk)
 
 
-def get_defined_list_rows_for_container_version(
-    container_version: ContainerEntityVersion,
-) -> QuerySet[EntityListRow]:
-    """
-    Get the user-defined members of a container version.
-
-    Args:
-        container_version: The container version to get the members of.
-
-    Returns:
-        The members of the container version.
-    """
-    return container_version.defined_list.entitylistrow_set.all()
-
-
-def get_initial_list_rows_for_container_version(
-    container_version: ContainerEntityVersion,
-) -> QuerySet[EntityListRow]:
-    """
-    Get the initial members of a container version.
-
-    Args:
-        container_version: The container version to get the initial members of.
-
-    Returns:
-        The initial members of the container version.
-    """
-    return container_version.initial_list.entitylistrow_set.all()
-
-
-def get_frozen_list_rows_for_container_version(
-    container_version: ContainerEntityVersion,
-) -> QuerySet[EntityListRow]:
-    """
-    Get the frozen members of a container version.
-
-    Args:
-        container_version: The container version to get the frozen members of.
-
-    Returns:
-        The frozen members of the container version.
-    """
-    if container_version.frozen_list is None:
-        return QuerySet[EntityListRow]()
-    return container_version.frozen_list.entitylistrow_set.all()
-
-
 @dataclass(frozen=True)
 class ContainerEntityListEntry:
     """
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 597cc5d1..e4073ac9 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -24,9 +24,6 @@
     "get_unit",
     "get_unit_version",
     "get_latest_unit_version",
-    "get_user_defined_list_in_unit_version",
-    "get_initial_list_in_unit_version",
-    "get_frozen_list_in_unit_version",
     "UnitListEntry",
     "get_components_in_draft_unit",
     "get_components_in_published_unit",
@@ -197,36 +194,6 @@ def get_latest_unit_version(unit_pk: int) -> UnitVersion:
     return Unit.objects.get(pk=unit_pk).versioning.latest
 
 
-def get_user_defined_list_in_unit_version(unit_version_pk: int) -> QuerySet[EntityListRow]:
-    """Get the list in a unit version.
-
-    Args:
-        unit_version_pk: The unit version ID.
-    """
-    unit_version = UnitVersion.objects.get(pk=unit_version_pk)
-    return container_api.get_defined_list_rows_for_container_version(unit_version.container_entity_version)
-
-
-def get_initial_list_in_unit_version(unit_version_pk: int) -> list[int]:
-    """Get the initial list in a unit version.
-
-    Args:
-        unit_version_pk: The unit version ID.
-    """
-    unit_version = UnitVersion.objects.get(pk=unit_version_pk)
-    return container_api.get_initial_list_rows_for_container_version(unit_version.container_entity_version)
-
-
-def get_frozen_list_in_unit_version(unit_version_pk: int) -> list[int]:
-    """Get the frozen list in a unit version.
-
-    Args:
-        unit_version_pk: The unit version ID.
-    """
-    unit_version = UnitVersion.objects.get(pk=unit_version_pk)
-    return container_api.get_frozen_list_rows_for_container_version(unit_version.container_entity_version)
-
-
 @dataclass(frozen=True)
 class UnitListEntry:
     """

From 57501472c3a766fa67a5b85f9c2924d8c442276a Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 17:59:04 -0800
Subject: [PATCH 09/27] docs: mark all containers APIs as unstable

---
 .../apps/authoring/containers/api.py          | 17 ++++++++++++
 openedx_learning/apps/authoring/units/api.py  | 26 ++++++++++++++-----
 .../apps/authoring/units/test_api.py          |  7 ++---
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index a186beb7..9ecf21b6 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -22,6 +22,8 @@
 from ..publishing import api as publishing_api
 
 
+# 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured
+#              out our approach to dynamic content (randomized, A/B tests, etc.)
 __all__ = [
     "create_container",
     "create_container_version",
@@ -42,6 +44,7 @@ def create_container(
     created_by: int | None,
 ) -> ContainerEntity:
     """
+    [ 🛑 UNSTABLE ]
     Create a new container.
 
     Args:
@@ -65,6 +68,7 @@ def create_container(
 
 def create_entity_list() -> EntityList:
     """
+    [ 🛑 UNSTABLE ]
     Create a new entity list. This is an structure that holds a list of entities
     that will be referenced by the container.
 
@@ -81,6 +85,7 @@ def create_next_defined_list(
     entity_version_pks: list[int | None],
 ) -> EntityListRow:
     """
+    [ 🛑 UNSTABLE ]
     Create new entity list rows for an entity list.
 
     Args:
@@ -129,6 +134,7 @@ def create_defined_list_with_rows(
     entity_version_pks: list[int | None],
 ) -> EntityList:
     """
+    [ 🛑 UNSTABLE ]
     Create new entity list rows for an entity list.
 
     Args:
@@ -164,6 +170,7 @@ def get_entity_list_with_pinned_versions(
     rows: QuerySet[EntityListRow],
 ) -> EntityList:
     """
+    [ 🛑 UNSTABLE ]
     Copy rows from an existing entity list to a new entity list.
 
     Args:
@@ -194,6 +201,7 @@ def check_unpinned_versions_in_defined_list(
     defined_list: EntityList,
 ) -> bool:
     """
+    [ 🛑 UNSTABLE ]
     Check if there are any unpinned versions in the defined list.
 
     Args:
@@ -214,6 +222,7 @@ def check_new_changes_in_defined_list(
     publishable_entities_pk: list[int],
 ) -> bool:
     """
+    [ 🛑 UNSTABLE ]
     Check if there are any new changes in the defined list.
 
     Args:
@@ -239,6 +248,7 @@ def create_container_version(
     created_by: int | None,
 ) -> ContainerEntityVersion:
     """
+    [ 🛑 UNSTABLE ]
     Create a new container version.
 
     Args:
@@ -288,6 +298,7 @@ def create_next_container_version(
     created_by: int | None,
 ) -> ContainerEntityVersion:
     """
+    [ 🛑 UNSTABLE ]
     Create the next version of a container. A new version of the container is created
     only when its metadata changes:
 
@@ -378,6 +389,7 @@ def create_container_and_version(
     entity_version_pks: list[int | None],
 ) -> ContainerEntityVersion:
     """
+    [ 🛑 UNSTABLE ]
     Create a new container and its first version.
 
     Args:
@@ -409,6 +421,7 @@ def create_container_and_version(
 
 def get_container(pk: int) -> ContainerEntity:
     """
+    [ 🛑 UNSTABLE ]
     Get a container by its primary key.
 
     Args:
@@ -424,6 +437,7 @@ def get_container(pk: int) -> ContainerEntity:
 @dataclass(frozen=True)
 class ContainerEntityListEntry:
     """
+    [ 🛑 UNSTABLE ]
     Data about a single entity in a container, e.g. a component in a unit.
     """
     entity_version: PublishableEntityVersion
@@ -438,6 +452,7 @@ def get_entities_in_draft_container(
     container: ContainerEntity | ContainerEntityMixin,
 ) -> list[ContainerEntityListEntry]:
     """
+    [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the draft version of the
     given container.
     """
@@ -458,6 +473,7 @@ def get_entities_in_published_container(
     container: ContainerEntity | ContainerEntityMixin,
 ) -> list[ContainerEntityListEntry] | None:
     """
+    [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the draft version of the
     given container.
     """
@@ -486,6 +502,7 @@ def contains_unpublished_changes(
     container: ContainerEntity | ContainerEntityMixin,
 ) -> bool:
     """
+    [ 🛑 UNSTABLE ]
     Check recursively if a container has any unpublished changes.
 
     Note: container.versioning.has_unpublished_changes only checks if the container
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index e4073ac9..dc90402c 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -16,6 +16,8 @@
 
 from datetime import datetime
 
+# 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured
+#              out our approach to dynamic content (randomized, A/B tests, etc.)
 __all__ = [
     "create_unit",
     "create_unit_version",
@@ -33,7 +35,8 @@
 def create_unit(
     learning_package_id: int, key: str, created: datetime, created_by: int | None
 ) -> Unit:
-    """Create a new unit.
+    """
+    [ 🛑 UNSTABLE ] Create a new unit.
 
     Args:
         learning_package_id: The learning package ID.
@@ -61,7 +64,8 @@ def create_unit_version(
     created: datetime,
     created_by: int | None = None,
 ) -> Unit:
-    """Create a new unit version.
+    """
+    [ 🛑 UNSTABLE ] Create a new unit version.
 
     Args:
         unit_pk: The unit ID.
@@ -98,7 +102,8 @@ def create_next_unit_version(
     created: datetime,
     created_by: int | None = None,
 ) -> Unit:
-    """Create the next unit version.
+    """
+    [ 🛑 UNSTABLE ] Create the next unit version.
 
     Args:
         unit_pk: The unit ID.
@@ -145,7 +150,8 @@ def create_unit_and_version(
     created: datetime,
     created_by: int | None = None,
 ) -> tuple[Unit, UnitVersion]:
-    """Create a new unit and its version.
+    """
+    [ 🛑 UNSTABLE ] Create a new unit and its version.
 
     Args:
         learning_package_id: The learning package ID.
@@ -168,7 +174,8 @@ def create_unit_and_version(
 
 
 def get_unit(unit_pk: int) -> Unit:
-    """Get a unit.
+    """
+    [ 🛑 UNSTABLE ] Get a unit.
 
     Args:
         unit_pk: The unit ID.
@@ -177,7 +184,8 @@ def get_unit(unit_pk: int) -> Unit:
 
 
 def get_unit_version(unit_version_pk: int) -> UnitVersion:
-    """Get a unit version.
+    """
+    [ 🛑 UNSTABLE ] Get a unit version.
 
     Args:
         unit_version_pk: The unit version ID.
@@ -186,7 +194,8 @@ def get_unit_version(unit_version_pk: int) -> UnitVersion:
 
 
 def get_latest_unit_version(unit_pk: int) -> UnitVersion:
-    """Get the latest unit version.
+    """
+    [ 🛑 UNSTABLE ] Get the latest unit version.
 
     Args:
         unit_pk: The unit ID.
@@ -197,6 +206,7 @@ def get_latest_unit_version(unit_pk: int) -> UnitVersion:
 @dataclass(frozen=True)
 class UnitListEntry:
     """
+    [ 🛑 UNSTABLE ]
     Data about a single entity in a container, e.g. a component in a unit.
     """
     component_version: ComponentVersion
@@ -211,6 +221,7 @@ def get_components_in_draft_unit(
     unit: Unit,
 ) -> list[UnitListEntry]:
     """
+    [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the draft version of the
     given container.
     """
@@ -228,6 +239,7 @@ def get_components_in_published_unit(
     unit: Unit | UnitVersion,
 ) -> list[UnitListEntry]:
     """
+    [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the draft version of the
     given container.
     """
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index f67925dd..ad01f60a 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -305,18 +305,19 @@ def test_query_count_of_contains_unpublished_changes(self):
         with self.assertNumQueries(2):
             assert authoring_api.contains_unpublished_changes(unit) == True
 
+    # Test that soft-deleting a component doesn't show the unit as changed but does show it contains changes ?
     # Test that only components can be added to units
     # Test that components must be in the same learning package
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks
-    # Test that publishing a unit publishes its child components (either automatically or throws an exception if you
-    #     don't request the children be published together with the containeer)
+    # Test that publishing a unit publishes its child components automatically
     # Test that publishing a component does NOT publish changes to its parent unit
     # Test that I can get a history of a given unit and all its children, including children that aren't currently in
     #     the unit and excluding children that are only in other units.
     # Test that I can get a history of a given unit and its children, that includes changes made to the child components
     #     while they were part of the unit but excludes changes made to those children while they were not part of
     #     the unit. 🫣
-    # Test viewing old snapshots of units and components by passing in a timestamp to some get_historic_unit() API?
+    # Test viewing old snapshots of units and components by passing in a timestamp (or PublishLog PK) to a
+    #     get_historic_unit() API?
 
 
     def test_next_version_with_different_different_title(self):

From fd83bd67c13b6801629d13a41a07600bd3567a69 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 14 Feb 2025 18:50:52 -0800
Subject: [PATCH 10/27] fix: minor corrections to units/containers API

---
 openedx_learning/apps/authoring/containers/api.py       | 2 ++
 openedx_learning/apps/authoring/units/api.py            | 4 ++--
 tests/openedx_learning/apps/authoring/units/test_api.py | 2 --
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 9ecf21b6..d4fac884 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -481,6 +481,8 @@ def get_entities_in_published_container(
         cev = container.container_entity.versioning.published
     elif isinstance(container, ContainerEntity):
         cev = container.versioning.published
+    else:
+        raise TypeError(f"Expected ContainerEntity or ContainerEntityMixin; got {type(container)}")
     if cev == None:
         return None  # There is no published version of this container. Should this be an exception?
     assert isinstance(cev, ContainerEntityVersion)
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index dc90402c..1f756ee1 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -236,14 +236,14 @@ def get_components_in_draft_unit(
 
 
 def get_components_in_published_unit(
-    unit: Unit | UnitVersion,
+    unit: Unit,
 ) -> list[UnitListEntry]:
     """
     [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the draft version of the
     given container.
     """
-    assert isinstance(unit, (Unit, UnitVersion))
+    assert isinstance(unit, Unit)
     published_entities = container_api.get_entities_in_published_container(unit)
     if published_entities == None:
         return None  # There is no published version of this unit. Should this be an exception?
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index ad01f60a..a13333b9 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -276,7 +276,6 @@ def test_modify_pinned_component(self):
         assert authoring_api.get_components_in_draft_unit(unit) == expected_unit_contents
         assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
 
-
     def test_query_count_of_contains_unpublished_changes(self):
         """
         Checking for unpublished changes in a unit should require a fixed number
@@ -319,7 +318,6 @@ def test_query_count_of_contains_unpublished_changes(self):
     # Test viewing old snapshots of units and components by passing in a timestamp (or PublishLog PK) to a
     #     get_historic_unit() API?
 
-
     def test_next_version_with_different_different_title(self):
         """Test creating a unit version with a different title.
 

From a061722f1f8ed9819904fcbacc63235b2a9a8c7d Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sat, 15 Feb 2025 20:39:05 -0800
Subject: [PATCH 11/27] feat: API to get a historic published version of a unit

---
 .../apps/authoring/containers/api.py          |  6 --
 .../apps/authoring/publishing/api.py          | 15 ++++
 openedx_learning/apps/authoring/units/api.py  | 44 ++++++++++-
 .../apps/authoring/units/test_api.py          | 79 +++++++++++++++++--
 4 files changed, 132 insertions(+), 12 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index d4fac884..065c8a01 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -108,16 +108,10 @@ def create_next_defined_list(
         # 2. Only associate existent rows to the new entity list iff: the order is the same, the PublishableEntity is in entity_pks and versions are not pinned
         # 3. If the order is different for a row with the PublishableEntity, create new row with the same PublishableEntity for the new order
         # and associate the new row to the new entity list
-        current_rows = previous_entity_list.entitylistrow_set.all()
-        publishable_entities_in_rows = {row.entity.pk: row for row in current_rows}
         new_rows = []
         for order_num, entity_pk, entity_version_pk in zip(
             order_nums, entity_pks, entity_version_pks
         ):
-            row = publishable_entities_in_rows.get(entity_pk)
-            if row and row.order_num == order_num:
-                new_entity_list.entitylistrow_set.add(row)
-                continue
             new_rows.append(
                 EntityListRow(
                     entity_list=new_entity_list,
diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py
index 3facc891..1a61ed70 100644
--- a/openedx_learning/apps/authoring/publishing/api.py
+++ b/openedx_learning/apps/authoring/publishing/api.py
@@ -513,3 +513,18 @@ def filter_publishable_entities(
         entities = entities.filter(published__version__isnull=not has_published)
 
     return entities
+
+
+def get_published_version_as_of(entity_id: int, publish_log_id: int) -> PublishableEntityVersion | None:
+    """
+    Get the published version of the given entity, at a specific snapshot in the
+    history of this Learning Package, given by the PublishLog ID.
+
+    This is a semi-private function, only available to other apps in the
+    authoring package.
+    """
+    record = PublishLogRecord.objects.filter(
+        entity_id=entity_id,
+        publish_log_id__lte=publish_log_id,
+    ).order_by('-publish_log_id').first()
+    return record.new_version if record else None
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 1f756ee1..24d27278 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -9,6 +9,7 @@
 from openedx_learning.apps.authoring.components.models import Component, ComponentVersion
 from openedx_learning.apps.authoring.containers.models import EntityListRow
 from ..publishing import api as publishing_api
+from ..publishing.api import get_published_version_as_of
 from ..containers import api as container_api
 from .models import Unit, UnitVersion
 from django.db.models import QuerySet
@@ -29,6 +30,7 @@
     "UnitListEntry",
     "get_components_in_draft_unit",
     "get_components_in_published_unit",
+    "get_components_in_published_unit_as_of",
 ]
 
 
@@ -240,7 +242,7 @@ def get_components_in_published_unit(
 ) -> list[UnitListEntry]:
     """
     [ 🛑 UNSTABLE ]
-    Get the list of entities and their versions in the draft version of the
+    Get the list of entities and their versions in the published version of the
     given container.
     """
     assert isinstance(unit, Unit)
@@ -254,3 +256,43 @@ def get_components_in_published_unit(
         assert isinstance(component_version, ComponentVersion)
         entity_list.append(UnitListEntry(component_version=component_version, pinned=entry.pinned))
     return entity_list
+
+
+def get_components_in_published_unit_as_of(
+    unit: Unit,
+    publish_log_id: int,
+) -> list[UnitListEntry] | None:
+    """
+    [ 🛑 UNSTABLE ]
+    Get the list of entities and their versions in the published version of the
+    given container as of the given PublishLog version (which is essentially a
+    version for the entire learning package).
+
+    TODO: This API should be updated to also return the UnitVersion so we can
+          see the unit title and any other metadata from that point in time.
+    TODO: accept a publish log UUID, not just int ID?
+    TODO: move the implementation to be a generic 'containers' implementation
+          that this units function merely wraps.
+    TODO: optimize, perhaps by having the publishlog store a record of all
+          ancestors of every modified PublishableEntity in the publish.
+    """
+    assert isinstance(unit, Unit)
+    unit_pub_entity_version = get_published_version_as_of(unit.publishable_entity, publish_log_id)
+    if unit_pub_entity_version is None:
+        return None  # This unit was not published as of the given PublishLog ID.
+    unit_version = unit_pub_entity_version.unitversion
+
+    entity_list = []
+    rows = unit_version.container_entity_version.defined_list.entitylistrow_set.order_by("order_num")
+    for row in rows:
+        if row.entity_version is not None:
+            component_version = row.entity_version.componentversion
+            assert isinstance(component_version, ComponentVersion)
+            entity_list.append(UnitListEntry(component_version=component_version, pinned=True))
+        else:
+            # Unpinned component - figure out what its latest published version was.
+            # This is not optimized. It could be done in one query per unit rather than one query per component.
+            pub_entity_version = get_published_version_as_of(row.entity, publish_log_id)
+            if pub_entity_version:
+                entity_list.append(UnitListEntry(component_version=pub_entity_version.componentversion, pinned=False))
+    return entity_list
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index a13333b9..25643c21 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -133,7 +133,7 @@ def test_create_next_unit_version_with_two_unpinned_components(self):
 
     def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         """
-        Test creating a unit version with one unpinned and one pinned component.
+        Test creating a unit version with one unpinned and one pinned 📌 component.
         """
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
@@ -145,7 +145,7 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         unit_version_v2 = authoring_api.create_next_unit_version(
             unit=unit,
             title="Unit",
-            components=[self.component_1, self.component_2_v1],  # Note the "v1" pinning it to version 2
+            components=[self.component_1, self.component_2_v1],  # Note the "v1" pinning 📌 the second one to version 1
             created=self.now,
             created_by=None,
         )
@@ -153,7 +153,7 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         assert unit_version_v2 in unit.versioning.versions.all()
         assert authoring_api.get_components_in_draft_unit(unit) == [
             authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=False),
-            authoring_api.UnitListEntry(component_version=self.component_2_v1, pinned=True),  # Pinned to v1
+            authoring_api.UnitListEntry(component_version=self.component_2_v1, pinned=True),  # Pinned 📌 to v1
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
 
@@ -304,6 +304,7 @@ def test_query_count_of_contains_unpublished_changes(self):
         with self.assertNumQueries(2):
             assert authoring_api.contains_unpublished_changes(unit) == True
 
+    # Test the query counts of various operations
     # Test that soft-deleting a component doesn't show the unit as changed but does show it contains changes ?
     # Test that only components can be added to units
     # Test that components must be in the same learning package
@@ -315,8 +316,76 @@ def test_query_count_of_contains_unpublished_changes(self):
     # Test that I can get a history of a given unit and its children, that includes changes made to the child components
     #     while they were part of the unit but excludes changes made to those children while they were not part of
     #     the unit. 🫣
-    # Test viewing old snapshots of units and components by passing in a timestamp (or PublishLog PK) to a
-    #     get_historic_unit() API?
+
+    def test_snapshots_of_published_unit(self):
+        """
+        Test that we can access snapshots of the historic published version of
+        units and their contents.
+        """
+        # At first the unit has one component (unpinned):
+        unit = self.create_unit_with_components([self.component_1])
+        self.modify_component(self.component_1, title="Component 1 as of checkpoint 1")
+
+        # Publish everything, creating Checkpoint 1
+        checkpoint_1 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 1")
+
+        ########################################################################
+
+        # Now we update the title of the component.
+        self.modify_component(self.component_1, title="Component 1 as of checkpoint 2")
+        # Publish everything, creating Checkpoint 2
+        checkpoint_2 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 2")
+        ########################################################################
+
+        # Now add a second component to the unit:
+        self.modify_component(self.component_1, title="Component 1 as of checkpoint 3")
+        self.modify_component(self.component_2, title="Component 2 as of checkpoint 3")
+        authoring_api.create_next_unit_version(
+            unit=unit,
+            title="Unit title in checkpoint 3",
+            components=[self.component_1, self.component_2],
+            created=self.now,
+        )
+        # Publish everything, creating Checkpoint 3
+        checkpoint_3 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 3")
+        ########################################################################
+
+        # Now add a third component to the unit, a pinned 📌 version of component 1.
+        # This will test pinned versions and also test adding at the beginning rather than the end of the unit.
+        authoring_api.create_next_unit_version(
+            unit=unit,
+            title="Unit title in checkpoint 4",
+            components=[self.component_1_v1, self.component_1, self.component_2],
+            created=self.now,
+        )
+        # Publish everything, creating Checkpoint 4
+        checkpoint_4 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 4")
+        ########################################################################
+
+        # Modify the drafts, but don't publish:
+        self.modify_component(self.component_1, title="Component 1 draft")
+        self.modify_component(self.component_2, title="Component 2 draft")
+
+        # Now fetch the snapshots:
+        as_of_checkpoint_1 = authoring_api.get_components_in_published_unit_as_of(unit, checkpoint_1.pk)
+        assert [cv.component_version.title for cv in as_of_checkpoint_1] == [
+            "Component 1 as of checkpoint 1",
+        ]
+        as_of_checkpoint_2 = authoring_api.get_components_in_published_unit_as_of(unit, checkpoint_2.pk)
+        assert [cv.component_version.title for cv in as_of_checkpoint_2] == [
+            "Component 1 as of checkpoint 2",
+        ]
+        as_of_checkpoint_3 = authoring_api.get_components_in_published_unit_as_of(unit, checkpoint_3.pk)
+        assert [cv.component_version.title for cv in as_of_checkpoint_3] == [
+            "Component 1 as of checkpoint 3",
+            "Component 2 as of checkpoint 3",
+        ]
+        as_of_checkpoint_4 = authoring_api.get_components_in_published_unit_as_of(unit, checkpoint_4.pk)
+        assert [cv.component_version.title for cv in as_of_checkpoint_4] == [
+            "Querying Counting Problem",  # Pinned. This title is self.component_1_v1.title (original v1 title)
+            "Component 1 as of checkpoint 3",  # we didn't modify these components so they're same as in snapshot 3
+            "Component 2 as of checkpoint 3",  # we didn't modify these components so they're same as in snapshot 3
+        ]
 
     def test_next_version_with_different_different_title(self):
         """Test creating a unit version with a different title.

From 74247f3d9f03cc5600d0eb9e2babe4bdd8cd1ee5 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sun, 16 Feb 2025 14:16:19 -0800
Subject: [PATCH 12/27] test: verify that units can only contain Components,
 not other types

---
 openedx_learning/apps/authoring/units/api.py  |  3 +-
 .../apps/authoring/units/test_api.py          | 38 ++++++++++++++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 24d27278..31b110a0 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -116,7 +116,8 @@ def create_next_unit_version(
         created_by: The user who created the unit.
     """
     for c in components:
-        assert isinstance(c, (Component, ComponentVersion))
+        if not isinstance(c, (Component, ComponentVersion)):
+            raise TypeError("Unit components must be either Component or ComponentVersion.")
     publishable_entities_pks = [
         (c.publishable_entity_id if isinstance(c, Component) else c.component.publishable_entity_id)
         for c in components
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 25643c21..8b50468d 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -1,6 +1,8 @@
 """
 Basic tests for the units API.
 """
+import pytest
+
 from ..components.test_api import ComponentTestCase
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
@@ -69,13 +71,37 @@ def modify_component(
             created_by=None,
         )
 
-    def test_create_unit_with_content_instead_of_components(self):
-        """Test creating a unit with content instead of components.
-
-        Expected results:
-        1. An error is raised indicating the content restriction for units.
-        2. The unit is not created.
+    def test_create_unit_with_invalid_children(self):
         """
+        Verify that only components can be added to units, and a specific
+        exception is raised.
+        """
+        # Create two units:
+        unit, unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        unit2, _u2v1 = authoring_api.create_unit_and_version(
+            learning_package_id=self.learning_package.id,
+            key=f"unit:key2",
+            title="Unit 2",
+            created=self.now,
+            created_by=None,
+        )
+        # Try adding a Unit to a Unit
+        with pytest.raises(TypeError, match="Unit components must be either Component or ComponentVersion."):
+            authoring_api.create_next_unit_version(
+                unit=unit,
+                title="Unit Containing a Unit",
+                components=[unit2],
+                created=self.now,
+                created_by=None,
+            )
+        # Check that a new version was not created:
+        assert unit.versioning.draft == unit_version
 
     def test_create_empty_unit_and_version(self):
         """Test creating a unit with no components.

From e7e647269117fb70512659d52eb2b728d480234f Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sun, 16 Feb 2025 14:34:41 -0800
Subject: [PATCH 13/27] test: "creating two units with the same components"

---
 .../apps/authoring/units/test_api.py          | 79 +++++++++++++++----
 1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 8b50468d..26170aca 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -7,6 +7,8 @@
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
 
+Entry = authoring_api.UnitListEntry
+
 
 class UnitTestCase(ComponentTestCase):
 
@@ -152,8 +154,8 @@ def test_create_next_unit_version_with_two_unpinned_components(self):
         assert unit_version_v2.version_num == 2
         assert unit_version_v2 in unit.versioning.versions.all()
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=self.component_1.versioning.draft, pinned=False),
-            authoring_api.UnitListEntry(component_version=self.component_2.versioning.draft, pinned=False),
+            Entry(self.component_1.versioning.draft, pinned=False),
+            Entry(self.component_2.versioning.draft, pinned=False),
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
 
@@ -178,8 +180,8 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         assert unit_version_v2.version_num == 2
         assert unit_version_v2 in unit.versioning.versions.all()
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=False),
-            authoring_api.UnitListEntry(component_version=self.component_2_v1, pinned=True),  # Pinned 📌 to v1
+            Entry(self.component_1_v1, pinned=False),
+            Entry(self.component_2_v1, pinned=True),  # Pinned 📌 to v1
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
 
@@ -252,19 +254,19 @@ def test_modify_unpinned_component_after_publish(self):
 
         # Since the component changes haven't been published, they should only appear in the draft unit
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2, pinned=False),  # new version
         ]
         assert authoring_api.get_components_in_published_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=False),  # old version
+            Entry(self.component_1_v1, pinned=False),  # old version
         ]
 
         # But if we publish the component, the changes will appear in the published version of the unit.
         self.publish_component(self.component_1)
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2, pinned=False),  # new version
         ]
         assert authoring_api.get_components_in_published_unit(unit) == [
-            authoring_api.UnitListEntry(component_version=component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2, pinned=False),  # new version
         ]
         assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
 
@@ -280,7 +282,7 @@ def test_modify_pinned_component(self):
         # Publish the unit and the component:
         authoring_api.publish_all_drafts(self.learning_package.id)
         expected_unit_contents = [
-            authoring_api.UnitListEntry(component_version=self.component_1_v1, pinned=True),  # pinned 📌 to v1
+            Entry(self.component_1_v1, pinned=True),  # pinned 📌 to v1
         ]
         assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
 
@@ -302,6 +304,57 @@ def test_modify_pinned_component(self):
         assert authoring_api.get_components_in_draft_unit(unit) == expected_unit_contents
         assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
 
+    def test_create_two_units_with_same_components(self):
+        """Test creating two units with the same components.
+
+        Expected results:
+        1. Two different units are created.
+        2. The units have the same components.
+        """
+        # Create a unit with component 2 unpinned, component 2 pinned 📌, and component 1:
+        unit1 = self.create_unit_with_components([self.component_2, self.component_2_v1, self.component_1], key="u1")
+        # Create a second unit with component 1 pinned 📌, component 2, and component 1 unpinned:
+        unit2 = self.create_unit_with_components([self.component_1_v1, self.component_2, self.component_1], key="u2")
+
+        # Check that the contents are as expected:
+        assert [row.component_version for row in authoring_api.get_components_in_draft_unit(unit1)] == [
+            self.component_2_v1, self.component_2_v1, self.component_1_v1,
+        ]
+        assert [row.component_version for row in authoring_api.get_components_in_draft_unit(unit2)] == [
+            self.component_1_v1, self.component_2_v1, self.component_1_v1,
+        ]
+
+        # Modify component 1
+        component_1_v2 = self.modify_component(self.component_1, title="component 1 v2")
+        # Publish changes
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        # Modify component 2 - only in the draft
+        component_2_v2 = self.modify_component(self.component_2, title="component 2 DRAFT")
+
+        # Check that the draft contents are as expected:
+        assert authoring_api.get_components_in_draft_unit(unit1) == [
+            Entry(component_2_v2, pinned=False),  # v2 in the draft version
+            Entry(self.component_2_v1, pinned=True),  # pinned 📌 to v1
+            Entry(component_1_v2, pinned=False),  # v2
+        ]
+        assert authoring_api.get_components_in_draft_unit(unit2) == [
+            Entry(self.component_1_v1, pinned=True),  # pinned 📌 to v1
+            Entry(component_2_v2, pinned=False),  # v2 in the draft version
+            Entry(component_1_v2, pinned=False),  # v2
+        ]
+
+        # Check that the published contents are as expected:
+        assert authoring_api.get_components_in_published_unit(unit1) == [
+            Entry(self.component_2_v1, pinned=False),  # v1 in the published version
+            Entry(self.component_2_v1, pinned=True),  # pinned 📌 to v1
+            Entry(component_1_v2, pinned=False),  # v2
+        ]
+        assert authoring_api.get_components_in_published_unit(unit2) == [
+            Entry(self.component_1_v1, pinned=True),  # pinned 📌 to v1
+            Entry(self.component_2_v1, pinned=False),  # v1 in the published version
+            Entry(component_1_v2, pinned=False),  # v2
+        ]
+
     def test_query_count_of_contains_unpublished_changes(self):
         """
         Checking for unpublished changes in a unit should require a fixed number
@@ -425,14 +478,6 @@ def test_next_version_with_different_different_title(self):
         6. The frozen list is empty.
         """
 
-    def test_create_two_units_with_same_components(self):
-        """Test creating two units with the same components.
-
-        Expected results:
-        1. Two different units are created.
-        2. The units have the same components.
-        """
-
     def test_check_author_defined_list_matches_components(self):
         """Test checking the author defined list matches the components.
 

From 6cc36c04df3d4a341a22726f44d21f043680e923 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 18 Feb 2025 10:37:59 -0800
Subject: [PATCH 14/27] test: verify that components are from the same learning
 package

---
 .../apps/authoring/containers/api.py          |  2 +-
 .../apps/authoring/units/test_api.py          | 26 +++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 065c8a01..f4ee44b8 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -298,7 +298,7 @@ def create_next_container_version(
 
     * Something was added to the Container.
     * We re-ordered the rows in the container.
-    * Something was removed to the container.
+    * Something was removed from the container.
     * The Container's metadata changed, e.g. the title.
     * We pin to different versions of the Container.
 
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 26170aca..67097af0 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -3,6 +3,8 @@
 """
 import pytest
 
+from django.core.exceptions import ValidationError
+
 from ..components.test_api import ComponentTestCase
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
@@ -105,6 +107,30 @@ def test_create_unit_with_invalid_children(self):
         # Check that a new version was not created:
         assert unit.versioning.draft == unit_version
 
+    def test_adding_external_components(self):
+        """
+        Test that components from another learning package cannot be added to a
+        unit.
+        """
+        learning_package2 = authoring_api.create_learning_package(key="other-package", title="Other Package")
+        unit, _unit_version = authoring_api.create_unit_and_version(
+            learning_package_id=learning_package2.pk,
+            key=f"unit:key",
+            title="Unit",
+            created=self.now,
+            created_by=None,
+        )
+        assert self.component_1.learning_package != learning_package2
+        # Try adding a a component from LP 1 (self.learning_package) to a unit from LP 2
+        with pytest.raises(ValidationError, match="Container entities must be from the same learning package."):
+            authoring_api.create_next_unit_version(
+                unit=unit,
+                title="Unit Containing an External Component",
+                components=[self.component_1],
+                created=self.now,
+                created_by=None,
+            )
+
     def test_create_empty_unit_and_version(self):
         """Test creating a unit with no components.
 

From c268dff42cc852aef8978edd057c996d24b248c3 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 18 Feb 2025 10:54:12 -0800
Subject: [PATCH 15/27] test: implement test_publishing_shared_component based
 on Slack convo

---
 .../apps/authoring/units/test_api.py          | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 67097af0..82a260df 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -381,6 +381,87 @@ def test_create_two_units_with_same_components(self):
             Entry(component_1_v2, pinned=False),  # v2
         ]
 
+    def test_publishing_shared_component(self):
+        """
+        A complex test case involving two units with a shared component and
+        other non-shared components.
+
+        Unit 1: components C1, C2, C3
+        Unit 2: components C2, C4, C5
+        Everything is "unpinned".
+        """
+        # 1️⃣ Create the units and publish them:
+        (c1, c1_v1), (c2, c2_v1), (c3, c3_v1), (c4, c4_v1), (c5, c5_v1) = [authoring_api.create_component_and_version(
+            self.learning_package.id,
+            component_type=self.problem_type,
+            local_key=f"C{i}",
+            title=f"Component {i}",
+            created=self.now,
+        ) for i in range(1, 6)]
+        unit1 = self.create_unit_with_components([c1, c2, c3], title="Unit 1", key="unit:1")
+        unit2 = self.create_unit_with_components([c2, c4, c5], title="Unit 2", key="unit:2")
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        assert authoring_api.contains_unpublished_changes(unit1) == False
+        assert authoring_api.contains_unpublished_changes(unit2) == False
+
+        # 2️⃣ Then the author edits C2 inside of Unit 1 making C2v2.
+        c2_v2 = self.modify_component(c2, title="C2 version 2")
+        # This makes U1 and U2 both show up as Units that CONTAIN unpublished changes, because they share the component.
+        assert authoring_api.contains_unpublished_changes(unit1)
+        assert authoring_api.contains_unpublished_changes(unit2)
+        # (But the units themselves are unchanged:)
+        unit1.refresh_from_db()
+        unit2.refresh_from_db()
+        assert unit1.versioning.has_unpublished_changes == False
+        assert unit2.versioning.has_unpublished_changes == False
+
+        # 3️⃣ In addition to this, the author also modifies another component in Unit 2 (C5)
+        c5_v2 = self.modify_component(c5, title="C5 version 2")
+
+        # 4️⃣ The author then publishes Unit 1, and therefore everything in it.
+        # FIXME: this should only require publishing the unit itself, but we don't yet do auto-publishing children
+        authoring_api.publish_from_drafts(
+            self.learning_package.pk,
+            draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter(
+                entity_id__in=[
+                    unit1.publishable_entity.id,
+                    c1.publishable_entity.id,
+                    c2.publishable_entity.id,
+                    c3.publishable_entity.id,
+                ],
+            ),
+        )
+
+        # Result: Unit 1 will show the newly published version of C2:
+        assert authoring_api.get_components_in_published_unit(unit1) == [
+            Entry(c1_v1, pinned=False),
+            Entry(c2_v2, pinned=False),  # new published version of C2
+            Entry(c3_v1, pinned=False),
+        ]
+
+        # Result: someone looking at Unit 2 should see the newly published component 2, because publishing it anywhere
+        # publishes it everywhere. But publishing C2 and Unit 1 does not affect the other components in Unit 2.
+        # (Publish propagates downward, not upward)
+        assert authoring_api.get_components_in_published_unit(unit2) == [
+            Entry(c2_v2, pinned=False),  # new published version of C2
+            Entry(c4_v1, pinned=False),  # still original version of C4 (it was never modified)
+            Entry(c5_v1, pinned=False),  # still original version of C5 (it hasn't been published)
+        ]
+
+        # Result: Unit 2 CONTAINS unpublished changes because of the modified C5. Unit 1 doesn't contain unpub changes.
+        assert authoring_api.contains_unpublished_changes(unit1) == False
+        assert authoring_api.contains_unpublished_changes(unit2)
+
+        # 5️⃣ Publish component C5, which should be the only thing unpublished in the learning package
+        self.publish_component(c5)
+        # Result: Unit 2 shows the new version of C5 and no longer contains unpublished changes:
+        assert authoring_api.get_components_in_published_unit(unit2) == [
+            Entry(c2_v2, pinned=False),  # new published version of C2
+            Entry(c4_v1, pinned=False),  # still original version of C4 (it was never modified)
+            Entry(c5_v2, pinned=False),  # new published version of C5
+        ]
+        assert authoring_api.contains_unpublished_changes(unit2) == False
+
     def test_query_count_of_contains_unpublished_changes(self):
         """
         Checking for unpublished changes in a unit should require a fixed number

From a6f21d3b97215554126ae7496309500801881aa0 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 18 Feb 2025 13:52:53 -0800
Subject: [PATCH 16/27] test: clean up unit tests by implying pinned+false by
 default

---
 openedx_learning/apps/authoring/units/api.py  |  2 +-
 .../apps/authoring/units/test_api.py          | 50 ++++++++++---------
 2 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 31b110a0..ad29e217 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -213,7 +213,7 @@ class UnitListEntry:
     Data about a single entity in a container, e.g. a component in a unit.
     """
     component_version: ComponentVersion
-    pinned: bool
+    pinned: bool = False
 
     @property
     def component(self):
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 82a260df..dcbde841 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -180,8 +180,8 @@ def test_create_next_unit_version_with_two_unpinned_components(self):
         assert unit_version_v2.version_num == 2
         assert unit_version_v2 in unit.versioning.versions.all()
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            Entry(self.component_1.versioning.draft, pinned=False),
-            Entry(self.component_2.versioning.draft, pinned=False),
+            Entry(self.component_1.versioning.draft),
+            Entry(self.component_2.versioning.draft),
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
 
@@ -206,7 +206,7 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         assert unit_version_v2.version_num == 2
         assert unit_version_v2 in unit.versioning.versions.all()
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            Entry(self.component_1_v1, pinned=False),
+            Entry(self.component_1_v1),
             Entry(self.component_2_v1, pinned=True),  # Pinned 📌 to v1
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
@@ -280,19 +280,19 @@ def test_modify_unpinned_component_after_publish(self):
 
         # Since the component changes haven't been published, they should only appear in the draft unit
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            Entry(component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2),  # new version
         ]
         assert authoring_api.get_components_in_published_unit(unit) == [
-            Entry(self.component_1_v1, pinned=False),  # old version
+            Entry(self.component_1_v1),  # old version
         ]
 
         # But if we publish the component, the changes will appear in the published version of the unit.
         self.publish_component(self.component_1)
         assert authoring_api.get_components_in_draft_unit(unit) == [
-            Entry(component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2),  # new version
         ]
         assert authoring_api.get_components_in_published_unit(unit) == [
-            Entry(component_1_v2, pinned=False),  # new version
+            Entry(component_1_v2),  # new version
         ]
         assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
 
@@ -359,26 +359,26 @@ def test_create_two_units_with_same_components(self):
 
         # Check that the draft contents are as expected:
         assert authoring_api.get_components_in_draft_unit(unit1) == [
-            Entry(component_2_v2, pinned=False),  # v2 in the draft version
+            Entry(component_2_v2),  # v2 in the draft version
             Entry(self.component_2_v1, pinned=True),  # pinned 📌 to v1
-            Entry(component_1_v2, pinned=False),  # v2
+            Entry(component_1_v2),  # v2
         ]
         assert authoring_api.get_components_in_draft_unit(unit2) == [
             Entry(self.component_1_v1, pinned=True),  # pinned 📌 to v1
-            Entry(component_2_v2, pinned=False),  # v2 in the draft version
-            Entry(component_1_v2, pinned=False),  # v2
+            Entry(component_2_v2),  # v2 in the draft version
+            Entry(component_1_v2),  # v2
         ]
 
         # Check that the published contents are as expected:
         assert authoring_api.get_components_in_published_unit(unit1) == [
-            Entry(self.component_2_v1, pinned=False),  # v1 in the published version
+            Entry(self.component_2_v1),  # v1 in the published version
             Entry(self.component_2_v1, pinned=True),  # pinned 📌 to v1
-            Entry(component_1_v2, pinned=False),  # v2
+            Entry(component_1_v2),  # v2
         ]
         assert authoring_api.get_components_in_published_unit(unit2) == [
             Entry(self.component_1_v1, pinned=True),  # pinned 📌 to v1
-            Entry(self.component_2_v1, pinned=False),  # v1 in the published version
-            Entry(component_1_v2, pinned=False),  # v2
+            Entry(self.component_2_v1),  # v1 in the published version
+            Entry(component_1_v2),  # v2
         ]
 
     def test_publishing_shared_component(self):
@@ -434,18 +434,18 @@ def test_publishing_shared_component(self):
 
         # Result: Unit 1 will show the newly published version of C2:
         assert authoring_api.get_components_in_published_unit(unit1) == [
-            Entry(c1_v1, pinned=False),
-            Entry(c2_v2, pinned=False),  # new published version of C2
-            Entry(c3_v1, pinned=False),
+            Entry(c1_v1),
+            Entry(c2_v2),  # new published version of C2
+            Entry(c3_v1),
         ]
 
         # Result: someone looking at Unit 2 should see the newly published component 2, because publishing it anywhere
         # publishes it everywhere. But publishing C2 and Unit 1 does not affect the other components in Unit 2.
         # (Publish propagates downward, not upward)
         assert authoring_api.get_components_in_published_unit(unit2) == [
-            Entry(c2_v2, pinned=False),  # new published version of C2
-            Entry(c4_v1, pinned=False),  # still original version of C4 (it was never modified)
-            Entry(c5_v1, pinned=False),  # still original version of C5 (it hasn't been published)
+            Entry(c2_v2),  # new published version of C2
+            Entry(c4_v1),  # still original version of C4 (it was never modified)
+            Entry(c5_v1),  # still original version of C5 (it hasn't been published)
         ]
 
         # Result: Unit 2 CONTAINS unpublished changes because of the modified C5. Unit 1 doesn't contain unpub changes.
@@ -456,9 +456,9 @@ def test_publishing_shared_component(self):
         self.publish_component(c5)
         # Result: Unit 2 shows the new version of C5 and no longer contains unpublished changes:
         assert authoring_api.get_components_in_published_unit(unit2) == [
-            Entry(c2_v2, pinned=False),  # new published version of C2
-            Entry(c4_v1, pinned=False),  # still original version of C4 (it was never modified)
-            Entry(c5_v2, pinned=False),  # new published version of C5
+            Entry(c2_v2),  # new published version of C2
+            Entry(c4_v1),  # still original version of C4 (it was never modified)
+            Entry(c5_v2),  # new published version of C5
         ]
         assert authoring_api.contains_unpublished_changes(unit2) == False
 
@@ -494,6 +494,8 @@ def test_query_count_of_contains_unpublished_changes(self):
     # Test that soft-deleting a component doesn't show the unit as changed but does show it contains changes ?
     # Test that only components can be added to units
     # Test that components must be in the same learning package
+    # Test that invalid component PKs cannot be added to a unit
+    # Test that soft-deleted components cannot be added to a unit?
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks
     # Test that publishing a unit publishes its child components automatically
     # Test that publishing a component does NOT publish changes to its parent unit

From 735f4e48e6f75d5531296fbc7308d172b130f076 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 19 Feb 2025 15:12:55 -0800
Subject: [PATCH 17/27] chore: fix minor quality issues

---
 .../apps/authoring/containers/api.py          | 23 +++---
 .../apps/authoring/containers/models.py       |  7 +-
 .../apps/authoring/containers/models_mixin.py |  7 +-
 openedx_learning/apps/authoring/units/api.py  |  8 +-
 .../apps/authoring/units/models.py            |  3 +
 .../apps/authoring/units/test_api.py          | 82 ++++++++++---------
 6 files changed, 65 insertions(+), 65 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index f4ee44b8..125f2cef 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -5,13 +5,12 @@
 models in the Open edX Learning platform.
 """
 from dataclasses import dataclass
+from datetime import datetime
 
 from django.db.transaction import atomic
 from django.db.models import QuerySet
 
-from datetime import datetime
-
-from openedx_learning.apps.authoring.containers.models_mixin import ContainerEntityMixin, ContainerEntityVersionMixin
+from openedx_learning.apps.authoring.containers.models_mixin import ContainerEntityMixin
 from ..containers.models import (
     ContainerEntity,
     ContainerEntityVersion,
@@ -79,7 +78,7 @@ def create_entity_list() -> EntityList:
 
 
 def create_next_defined_list(
-    previous_entity_list: EntityList | None,
+    previous_entity_list: EntityList | None,  # pylint: disable=unused-argument
     new_entity_list: EntityList,
     entity_pks: list[int],
     entity_version_pks: list[int | None],
@@ -105,9 +104,10 @@ def create_next_defined_list(
         # 1. Create new rows for the entity list
         # Case 2: create next container version (previous rows created for container)
         # 1. Get all the rows in the previous version entity list
-        # 2. Only associate existent rows to the new entity list iff: the order is the same, the PublishableEntity is in entity_pks and versions are not pinned
-        # 3. If the order is different for a row with the PublishableEntity, create new row with the same PublishableEntity for the new order
-        # and associate the new row to the new entity list
+        # 2. Only associate existent rows to the new entity list iff: the order is the same, the PublishableEntity is in
+        #    entity_pks and versions are not pinned
+        # 3. If the order is different for a row with the PublishableEntity, create new row with the same
+        #    PublishableEntity for the new order and associate the new row to the new entity list
         new_rows = []
         for order_num, entity_pk, entity_version_pk in zip(
             order_nums, entity_pks, entity_version_pks
@@ -212,8 +212,8 @@ def check_unpinned_versions_in_defined_list(
 
 
 def check_new_changes_in_defined_list(
-    entity_list: EntityList,
-    publishable_entities_pk: list[int],
+    entity_list: EntityList,  # pylint: disable=unused-argument
+    publishable_entities_pk: list[int],  # pylint: disable=unused-argument
 ) -> bool:
     """
     [ 🛑 UNSTABLE ]
@@ -477,7 +477,7 @@ def get_entities_in_published_container(
         cev = container.versioning.published
     else:
         raise TypeError(f"Expected ContainerEntity or ContainerEntityMixin; got {type(container)}")
-    if cev == None:
+    if cev is None:
         return None  # There is no published version of this container. Should this be an exception?
     assert isinstance(cev, ContainerEntityVersion)
     # TODO: do we ever need frozen_list? e.g. when accessing a historical version?
@@ -533,7 +533,7 @@ def contains_unpublished_changes(
     ):
         try:
             child_container = row.entity.containerentity
-        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:
+        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:  # pylint: disable=no-member
             child_container = None
         if child_container:
             child_container = row.entity.containerentity
@@ -546,5 +546,4 @@ def contains_unpublished_changes(
             published_pk = row.entity.published.version_id if row.entity.published else None
             if draft_pk != published_pk:
                 return True
-    
     return False
diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py
index be1ac0b6..8ae50936 100644
--- a/openedx_learning/apps/authoring/containers/models.py
+++ b/openedx_learning/apps/authoring/containers/models.py
@@ -1,3 +1,6 @@
+"""
+Models that implement containers
+"""
 from django.db import models
 
 from openedx_learning.apps.authoring.publishing.models import (
@@ -26,8 +29,6 @@ class EntityList(models.Model):
     other models, rather than being looked up by their own identifers.
     """
 
-    pass
-
 
 class EntityListRow(models.Model):
     """
@@ -78,8 +79,6 @@ class ContainerEntity(PublishableEntityMixin):
     child elements were published.
     """
 
-    pass
-
 
 class ContainerEntityVersion(PublishableEntityVersionMixin):
     """
diff --git a/openedx_learning/apps/authoring/containers/models_mixin.py b/openedx_learning/apps/authoring/containers/models_mixin.py
index 4accd119..90477566 100644
--- a/openedx_learning/apps/authoring/containers/models_mixin.py
+++ b/openedx_learning/apps/authoring/containers/models_mixin.py
@@ -1,14 +1,15 @@
+"""
+Mixins for models that implement containers
+"""
 from __future__ import annotations
 
 from django.db import models
+from django.db.models.query import QuerySet
 
 from openedx_learning.apps.authoring.containers.models import (
     ContainerEntity,
     ContainerEntityVersion,
 )
-
-from django.db.models.query import QuerySet
-
 from openedx_learning.apps.authoring.publishing.model_mixins import (
     PublishableEntityMixin,
     PublishableEntityVersionMixin,
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index ad29e217..d5d12d25 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -3,19 +3,15 @@
 This module provides functions to manage units.
 """
 from dataclasses import dataclass
+from datetime import datetime
 
 from django.db.transaction import atomic
 
 from openedx_learning.apps.authoring.components.models import Component, ComponentVersion
-from openedx_learning.apps.authoring.containers.models import EntityListRow
-from ..publishing import api as publishing_api
 from ..publishing.api import get_published_version_as_of
 from ..containers import api as container_api
 from .models import Unit, UnitVersion
-from django.db.models import QuerySet
-
 
-from datetime import datetime
 
 # 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured
 #              out our approach to dynamic content (randomized, A/B tests, etc.)
@@ -248,7 +244,7 @@ def get_components_in_published_unit(
     """
     assert isinstance(unit, Unit)
     published_entities = container_api.get_entities_in_published_container(unit)
-    if published_entities == None:
+    if published_entities is None:
         return None  # There is no published version of this unit. Should this be an exception?
     entity_list = []
     for entry in published_entities:
diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py
index 502b34a6..67919a63 100644
--- a/openedx_learning/apps/authoring/units/models.py
+++ b/openedx_learning/apps/authoring/units/models.py
@@ -1,3 +1,6 @@
+"""
+Models that implement units
+"""
 from django.db import models
 
 from ..containers.models_mixin import ContainerEntityMixin, ContainerEntityVersionMixin
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index dcbde841..c47c060f 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -5,16 +5,18 @@
 
 from django.core.exceptions import ValidationError
 
-from ..components.test_api import ComponentTestCase
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
+from ..components.test_api import ComponentTestCase
 
 Entry = authoring_api.UnitListEntry
 
 
 class UnitTestCase(ComponentTestCase):
+    """ Test cases for Units (containers of components) """
 
     def setUp(self) -> None:
+        super().setUp()
         self.component_1, self.component_1_v1 = authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
@@ -56,7 +58,7 @@ def create_unit_with_components(
         )
         unit.refresh_from_db()
         return unit
-    
+
     def modify_component(
         self,
         component: authoring_models.Component,
@@ -83,14 +85,14 @@ def test_create_unit_with_invalid_children(self):
         # Create two units:
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
         )
         unit2, _u2v1 = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key2",
+            key="unit:key2",
             title="Unit 2",
             created=self.now,
             created_by=None,
@@ -115,7 +117,7 @@ def test_adding_external_components(self):
         learning_package2 = authoring_api.create_learning_package(key="other-package", title="Other Package")
         unit, _unit_version = authoring_api.create_unit_and_version(
             learning_package_id=learning_package2.pk,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
@@ -142,7 +144,7 @@ def test_create_empty_unit_and_version(self):
         """
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
@@ -150,7 +152,7 @@ def test_create_empty_unit_and_version(self):
         assert unit, unit_version
         assert unit_version.version_num == 1
         assert unit_version in unit.versioning.versions.all()
-        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes
         assert unit.versioning.draft == unit_version
         assert unit.versioning.published is None
 
@@ -163,9 +165,9 @@ def test_create_next_unit_version_with_two_unpinned_components(self):
         3. The unit version is in the unit's versions.
         4. The components are in the draft unit version's component list and are unpinned.
         """
-        unit, unit_version = authoring_api.create_unit_and_version(
+        unit, _unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
@@ -189,9 +191,9 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         """
         Test creating a unit version with one unpinned and one pinned 📌 component.
         """
-        unit, unit_version = authoring_api.create_unit_and_version(
+        unit, _unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
@@ -218,22 +220,22 @@ def test_add_component_after_publish(self):
         """
         unit, unit_version = authoring_api.create_unit_and_version(
             learning_package_id=self.learning_package.id,
-            key=f"unit:key",
+            key="unit:key",
             title="Unit",
             created=self.now,
             created_by=None,
         )
         assert unit.versioning.draft == unit_version
         assert unit.versioning.published is None
-        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes
         # Publish the empty unit:
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()  # Reloading the unit is necessary
-        assert unit.versioning.has_unpublished_changes == False  # Shallow check for just the unit itself, not children
-        assert authoring_api.contains_unpublished_changes(unit) == False  # Deeper check
+        assert unit.versioning.has_unpublished_changes is False  # Shallow check for just the unit itself, not children
+        assert authoring_api.contains_unpublished_changes(unit) is False  # Deeper check
 
         # Add a published component (unpinned):
-        assert self.component_1.versioning.has_unpublished_changes == False
+        assert self.component_1.versioning.has_unpublished_changes is False
         unit_version_v2 = authoring_api.create_next_unit_version(
             unit=unit,
             title=unit_version.title,
@@ -243,8 +245,8 @@ def test_add_component_after_publish(self):
         )
         # Now the unit should have unpublished changes:
         unit.refresh_from_db()  # Reloading the unit is necessary
-        assert unit.versioning.has_unpublished_changes == True  # Shallow check - adding a child is a change to the unit
-        assert authoring_api.contains_unpublished_changes(unit) == True  # Deeper check
+        assert unit.versioning.has_unpublished_changes  # Shallow check - adding a child is a change to the unit
+        assert authoring_api.contains_unpublished_changes(unit)  # Deeper check
         assert unit.versioning.draft == unit_version_v2
         assert unit.versioning.published == unit_version
 
@@ -256,17 +258,17 @@ def test_modify_unpinned_component_after_publish(self):
         published version of the unit only after the component is published.
         """
         # Create a unit with one unpinned draft component:
-        assert self.component_1.versioning.has_unpublished_changes == True
+        assert self.component_1.versioning.has_unpublished_changes
         unit = self.create_unit_with_components([self.component_1])
-        assert unit.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes
 
         # Publish the unit and the component:
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()  # Reloading the unit is necessary if we accessed 'versioning' before publish
         self.component_1.refresh_from_db()
-        assert unit.versioning.has_unpublished_changes == False  # Shallow check
-        assert authoring_api.contains_unpublished_changes(unit) == False  # Deeper check
-        assert self.component_1.versioning.has_unpublished_changes == False
+        assert unit.versioning.has_unpublished_changes is False  # Shallow check
+        assert authoring_api.contains_unpublished_changes(unit) is False  # Deeper check
+        assert self.component_1.versioning.has_unpublished_changes is False
 
         # Now modify the component by changing its title (it remains a draft):
         component_1_v2 = self.modify_component(self.component_1, title="Modified Counting Problem with new title")
@@ -274,9 +276,9 @@ def test_modify_unpinned_component_after_publish(self):
         # The component now has unpublished changes; the unit doesn't directly but does contain
         unit.refresh_from_db()  # Reloading the unit is necessary, or 'unit.versioning' will be outdated
         self.component_1.refresh_from_db()
-        assert unit.versioning.has_unpublished_changes == False  # Shallow check should be false - unit is unchanged
-        assert authoring_api.contains_unpublished_changes(unit) == True  # But unit DOES contain changes
-        assert self.component_1.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes is False  # Shallow check should be false - unit is unchanged
+        assert authoring_api.contains_unpublished_changes(unit)  # But unit DOES contain changes
+        assert self.component_1.versioning.has_unpublished_changes
 
         # Since the component changes haven't been published, they should only appear in the draft unit
         assert authoring_api.get_components_in_draft_unit(unit) == [
@@ -294,7 +296,7 @@ def test_modify_unpinned_component_after_publish(self):
         assert authoring_api.get_components_in_published_unit(unit) == [
             Entry(component_1_v2),  # new version
         ]
-        assert authoring_api.contains_unpublished_changes(unit) == False  # No longer contains unpublished changes
+        assert authoring_api.contains_unpublished_changes(unit) is False  # No longer contains unpublished changes
 
     def test_modify_pinned_component(self):
         """
@@ -318,9 +320,9 @@ def test_modify_pinned_component(self):
         # The component now has unpublished changes; the unit is entirely unaffected
         unit.refresh_from_db()  # Reloading the unit is necessary, or 'unit.versioning' will be outdated
         self.component_1.refresh_from_db()
-        assert unit.versioning.has_unpublished_changes == False  # Shallow check
-        assert authoring_api.contains_unpublished_changes(unit) == False  # Deep check
-        assert self.component_1.versioning.has_unpublished_changes == True
+        assert unit.versioning.has_unpublished_changes is False  # Shallow check
+        assert authoring_api.contains_unpublished_changes(unit) is False  # Deep check
+        assert self.component_1.versioning.has_unpublished_changes is True
 
         # Neither the draft nor the published version of the unit is affected
         assert authoring_api.get_components_in_draft_unit(unit) == expected_unit_contents
@@ -391,7 +393,7 @@ def test_publishing_shared_component(self):
         Everything is "unpinned".
         """
         # 1️⃣ Create the units and publish them:
-        (c1, c1_v1), (c2, c2_v1), (c3, c3_v1), (c4, c4_v1), (c5, c5_v1) = [authoring_api.create_component_and_version(
+        (c1, c1_v1), (c2, _c2_v1), (c3, c3_v1), (c4, c4_v1), (c5, c5_v1) = [authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
             local_key=f"C{i}",
@@ -401,8 +403,8 @@ def test_publishing_shared_component(self):
         unit1 = self.create_unit_with_components([c1, c2, c3], title="Unit 1", key="unit:1")
         unit2 = self.create_unit_with_components([c2, c4, c5], title="Unit 2", key="unit:2")
         authoring_api.publish_all_drafts(self.learning_package.id)
-        assert authoring_api.contains_unpublished_changes(unit1) == False
-        assert authoring_api.contains_unpublished_changes(unit2) == False
+        assert authoring_api.contains_unpublished_changes(unit1) is False
+        assert authoring_api.contains_unpublished_changes(unit2) is False
 
         # 2️⃣ Then the author edits C2 inside of Unit 1 making C2v2.
         c2_v2 = self.modify_component(c2, title="C2 version 2")
@@ -412,8 +414,8 @@ def test_publishing_shared_component(self):
         # (But the units themselves are unchanged:)
         unit1.refresh_from_db()
         unit2.refresh_from_db()
-        assert unit1.versioning.has_unpublished_changes == False
-        assert unit2.versioning.has_unpublished_changes == False
+        assert unit1.versioning.has_unpublished_changes is False
+        assert unit2.versioning.has_unpublished_changes is False
 
         # 3️⃣ In addition to this, the author also modifies another component in Unit 2 (C5)
         c5_v2 = self.modify_component(c5, title="C5 version 2")
@@ -449,7 +451,7 @@ def test_publishing_shared_component(self):
         ]
 
         # Result: Unit 2 CONTAINS unpublished changes because of the modified C5. Unit 1 doesn't contain unpub changes.
-        assert authoring_api.contains_unpublished_changes(unit1) == False
+        assert authoring_api.contains_unpublished_changes(unit1) is False
         assert authoring_api.contains_unpublished_changes(unit2)
 
         # 5️⃣ Publish component C5, which should be the only thing unpublished in the learning package
@@ -460,7 +462,7 @@ def test_publishing_shared_component(self):
             Entry(c4_v1),  # still original version of C4 (it was never modified)
             Entry(c5_v2),  # new published version of C5
         ]
-        assert authoring_api.contains_unpublished_changes(unit2) == False
+        assert authoring_api.contains_unpublished_changes(unit2) is False
 
     def test_query_count_of_contains_unpublished_changes(self):
         """
@@ -470,7 +472,7 @@ def test_query_count_of_contains_unpublished_changes(self):
         # Add 100 components (unpinned)
         component_count = 100
         components = []
-        for i in range(0, component_count):
+        for i in range(component_count):
             component, _version = authoring_api.create_component_and_version(
                 self.learning_package.id,
                 component_type=self.problem_type,
@@ -483,12 +485,12 @@ def test_query_count_of_contains_unpublished_changes(self):
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()
         with self.assertNumQueries(3):
-            assert authoring_api.contains_unpublished_changes(unit) == False
+            assert authoring_api.contains_unpublished_changes(unit) is False
 
         # Modify the most recently created component:
         self.modify_component(component, title="Modified Component")
         with self.assertNumQueries(2):
-            assert authoring_api.contains_unpublished_changes(unit) == True
+            assert authoring_api.contains_unpublished_changes(unit) is True
 
     # Test the query counts of various operations
     # Test that soft-deleting a component doesn't show the unit as changed but does show it contains changes ?

From 8f271927965254aad5491e57c76a04a1a80b2217 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 19 Feb 2025 15:27:47 -0800
Subject: [PATCH 18/27] refactor: re-enable the too-many-positional-arguments
 pylint check

---
 .../apps/authoring/components/api.py          |  4 +--
 .../apps/authoring/containers/api.py          | 23 +++++++++-------
 openedx_learning/apps/authoring/units/api.py  | 27 ++++++++++---------
 openedx_tagging/core/tagging/api.py           |  4 +--
 openedx_tagging/core/tagging/models/base.py   |  2 +-
 pylintrc                                      |  1 -
 pylintrc_tweaks                               |  1 -
 .../core/tagging/test_views.py                | 19 ++++++-------
 8 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py
index 48a45af4..7f5bb33e 100644
--- a/openedx_learning/apps/authoring/components/api.py
+++ b/openedx_learning/apps/authoring/components/api.py
@@ -231,7 +231,7 @@ def create_next_component_version(
         return component_version
 
 
-def create_component_and_version(
+def create_component_and_version(  # pylint: disable=too-many-positional-arguments
     learning_package_id: int,
     /,
     component_type: ComponentType,
@@ -326,7 +326,7 @@ def component_exists_by_key(
         return False
 
 
-def get_components(
+def get_components(  # pylint: disable=too-many-positional-arguments
     learning_package_id: int,
     /,
     draft: bool | None = None,
diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 125f2cef..e39fc93b 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -213,7 +213,7 @@ def check_unpinned_versions_in_defined_list(
 
 def check_new_changes_in_defined_list(
     entity_list: EntityList,  # pylint: disable=unused-argument
-    publishable_entities_pk: list[int],  # pylint: disable=unused-argument
+    publishable_entities_pks: list[int],  # pylint: disable=unused-argument
 ) -> bool:
     """
     [ 🛑 UNSTABLE ]
@@ -234,8 +234,9 @@ def check_new_changes_in_defined_list(
 def create_container_version(
     container_pk: int,
     version_num: int,
+    *,
     title: str,
-    publishable_entities_pk: list[int],
+    publishable_entities_pks: list[int],
     entity_version_pks: list[int | None],
     entity: PublishableEntity,
     created: datetime,
@@ -249,7 +250,7 @@ def create_container_version(
         container_pk: The ID of the container that the version belongs to.
         version_num: The version number of the container.
         title: The title of the container.
-        publishable_entities_pk: The IDs of the members of the container.
+        publishable_entities_pks: The IDs of the members of the container.
         entity: The entity that the container belongs to.
         created: The date and time the container version was created.
         created_by: The ID of the user who created the container version.
@@ -266,7 +267,7 @@ def create_container_version(
             created_by=created_by,
         )
         defined_list = create_defined_list_with_rows(
-            entity_pks=publishable_entities_pk,
+            entity_pks=publishable_entities_pks,
             entity_version_pks=entity_version_pks,
         )
         container_version = ContainerEntityVersion.objects.create(
@@ -284,8 +285,9 @@ def create_container_version(
 
 def create_next_container_version(
     container_pk: int,
+    *,
     title: str,
-    publishable_entities_pk: list[int],
+    publishable_entities_pks: list[int],
     entity_version_pks: list[int | None],
     entity: PublishableEntity,
     created: datetime,
@@ -305,7 +307,7 @@ def create_next_container_version(
     Args:
         container_pk: The ID of the container to create the next version of.
         title: The title of the container.
-        publishable_entities_pk: The IDs of the members current members of the container.
+        publishable_entities_pks: The IDs of the members current members of the container.
         entity: The entity that the container belongs to.
         created: The date and time the container version was created.
         created_by: The ID of the user who created the container version.
@@ -332,7 +334,7 @@ def create_next_container_version(
         # 6. Point frozen_list to None or defined_list
         if check_new_changes_in_defined_list(
             entity_list=last_version.defined_list,
-            publishable_entities_pk=publishable_entities_pk,
+            publishable_entities_pks=publishable_entities_pks,
         ):
             # Only change if there are unpin versions in defined list, meaning last frozen list is None
             # When does this has to happen? Before?
@@ -344,7 +346,7 @@ def create_next_container_version(
             next_defined_list = create_next_defined_list(
                 previous_entity_list=last_version.defined_list,
                 new_entity_list=create_entity_list(),
-                entity_pks=publishable_entities_pk,
+                entity_pks=publishable_entities_pks,
                 entity_version_pks=entity_version_pks,
             )
             next_initial_list = get_entity_list_with_pinned_versions(
@@ -376,10 +378,11 @@ def create_next_container_version(
 def create_container_and_version(
     learning_package_id: int,
     key: str,
+    *,
     created: datetime,
     created_by: int | None,
     title: str,
-    publishable_entities_pk: list[int],
+    publishable_entities_pks: list[int],
     entity_version_pks: list[int | None],
 ) -> ContainerEntityVersion:
     """
@@ -404,7 +407,7 @@ def create_container_and_version(
             container_pk=container.publishable_entity.pk,
             version_num=1,
             title=title,
-            publishable_entities_pk=publishable_entities_pk,
+            publishable_entities_pks=publishable_entities_pks,
             entity_version_pks=entity_version_pks,
             entity=container.publishable_entity,
             created=created,
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index d5d12d25..9d484f0b 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -56,6 +56,7 @@ def create_unit(
 def create_unit_version(
     unit: Unit,
     version_num: int,
+    *,
     title: str,
     publishable_entities_pks: list[int],
     entity_version_pks: list[int | None],
@@ -78,12 +79,12 @@ def create_unit_version(
         container_entity_version = container_api.create_container_version(
             unit.container_entity.pk,
             version_num,
-            title,
-            publishable_entities_pks,
-            entity_version_pks,
-            unit.container_entity.publishable_entity,
-            created,
-            created_by,
+            title=title,
+            publishable_entities_pks=publishable_entities_pks,
+            entity_version_pks=entity_version_pks,
+            entity=unit.container_entity.publishable_entity,
+            created=created,
+            created_by=created_by,
         )
         unit_version = UnitVersion.objects.create(
             unit=unit,
@@ -127,12 +128,12 @@ def create_next_unit_version(
         # This currently allows for any publishable entity to be added to a unit.
         container_entity_version = container_api.create_next_container_version(
             unit.container_entity.pk,
-            title,
-            publishable_entities_pks,
-            entity_version_pks,
-            unit.container_entity.publishable_entity,
-            created,
-            created_by,
+            title=title,
+            publishable_entities_pks=publishable_entities_pks,
+            entity_version_pks=entity_version_pks,
+            entity=unit.container_entity.publishable_entity,
+            created=created,
+            created_by=created_by,
         )
         unit_version = UnitVersion.objects.create(
             unit=unit,
@@ -163,7 +164,7 @@ def create_unit_and_version(
         unit_version = create_unit_version(
             unit,
             1,
-            title,
+            title=title,
             publishable_entities_pks=[],
             entity_version_pks=[],
             created=created,
diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py
index 8c03c740..e4283005 100644
--- a/openedx_tagging/core/tagging/api.py
+++ b/openedx_tagging/core/tagging/api.py
@@ -28,7 +28,7 @@
 TagDoesNotExist = Tag.DoesNotExist
 
 
-def create_taxonomy(
+def create_taxonomy(  # pylint: disable=too-many-positional-arguments
     name: str,
     description: str | None = None,
     enabled=True,
@@ -321,7 +321,7 @@ def _get_current_tags(
     return current_tags
 
 
-def tag_object(
+def tag_object(  # pylint: disable=too-many-positional-arguments
     object_id: str,
     taxonomy: Taxonomy | None,
     tags: list[str],
diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py
index 6b2cccf9..aa2ef407 100644
--- a/openedx_tagging/core/tagging/models/base.py
+++ b/openedx_tagging/core/tagging/models/base.py
@@ -382,7 +382,7 @@ def copy(self, taxonomy: Taxonomy) -> Taxonomy:
 
         return self
 
-    def get_filtered_tags(
+    def get_filtered_tags(  # pylint: disable=too-many-positional-arguments
         self,
         depth: int | None = TAXONOMY_MAX_DEPTH,
         parent_tag_value: str | None = None,
diff --git a/pylintrc b/pylintrc
index b56bdeec..42cd7f9d 100644
--- a/pylintrc
+++ b/pylintrc
@@ -290,7 +290,6 @@ disable =
 	invalid-name,
 	django-not-configured,
 	consider-using-with,
-	too-many-positional-arguments,
 
 [REPORTS]
 output-format = text
diff --git a/pylintrc_tweaks b/pylintrc_tweaks
index 8eb879bd..f999b169 100644
--- a/pylintrc_tweaks
+++ b/pylintrc_tweaks
@@ -8,4 +8,3 @@ disable+=
     invalid-name,
     django-not-configured,
     consider-using-with,
-    too-many-positional-arguments,
diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py
index 936ba0a3..7b24a1f6 100644
--- a/tests/openedx_tagging/core/tagging/test_views.py
+++ b/tests/openedx_tagging/core/tagging/test_views.py
@@ -45,8 +45,9 @@
 
 def check_taxonomy(
     data,
-    taxonomy_id,
-    name,
+    *,
+    taxonomy_id: int,
+    name: str,
     description="",
     enabled=True,
     allow_multiple=True,
@@ -333,7 +334,7 @@ def test_detail_taxonomy(
             expected_data["can_change_taxonomy"] = is_admin
             expected_data["can_delete_taxonomy"] = is_admin
             expected_data["can_tag_object"] = False
-            check_taxonomy(response.data, taxonomy.pk, **expected_data)
+            check_taxonomy(response.data, taxonomy_id=taxonomy.pk, **expected_data)  # type: ignore[arg-type]
 
     def test_detail_system_taxonomy(self):
         url = TAXONOMY_DETAIL_URL.format(pk=LANGUAGE_TAXONOMY_ID)
@@ -385,11 +386,11 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int):
             create_data["can_change_taxonomy"] = True
             create_data["can_delete_taxonomy"] = True
             create_data["can_tag_object"] = False
-            check_taxonomy(response.data, response.data["id"], **create_data)
+            check_taxonomy(response.data, taxonomy_id=response.data["id"], **create_data)  # type: ignore[arg-type]
             url = TAXONOMY_DETAIL_URL.format(pk=response.data["id"])
 
             response = self.client.get(url)
-            check_taxonomy(response.data, response.data["id"], **create_data)
+            check_taxonomy(response.data, taxonomy_id=response.data["id"], **create_data)  # type: ignore[arg-type]
 
     def test_create_without_export_id(self):
         url = TAXONOMY_LIST_URL
@@ -409,7 +410,7 @@ def test_create_without_export_id(self):
         create_data["can_tag_object"] = False
         check_taxonomy(
             response.data,
-            response.data["id"],
+            taxonomy_id=response.data["id"],
             export_id="2-taxonomy-data-3",
             **create_data,
         )
@@ -465,7 +466,7 @@ def test_update_taxonomy(self, user_attr, expected_status):
             response = self.client.get(url)
             check_taxonomy(
                 response.data,
-                response.data["id"],
+                taxonomy_id=response.data["id"],
                 **{
                     "name": "new name",
                     "description": "taxonomy description",
@@ -526,7 +527,7 @@ def test_patch_taxonomy(self, user_attr, expected_status):
             response = self.client.get(url)
             check_taxonomy(
                 response.data,
-                response.data["id"],
+                taxonomy_id=response.data["id"],
                 **{
                     "name": "new name",
                     "enabled": True,
@@ -1041,7 +1042,7 @@ def test_object_tags_remaining_http_methods(
         ("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"),
     )
     @ddt.unpack
-    def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status, object_id):
+    def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status, object_id):  # pylint: disable=too-many-positional-arguments
         if user_attr:
             user = getattr(self, user_attr)
             self.client.force_authenticate(user=user)

From 4f5002201920143beac7f81f38d319a880f2e147 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 19 Feb 2025 16:15:35 -0800
Subject: [PATCH 19/27] fix: reduce query count of contains_unpublished_changes

---
 openedx_learning/apps/authoring/containers/api.py       | 2 +-
 tests/openedx_learning/apps/authoring/units/test_api.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index e39fc93b..f8c37944 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -515,7 +515,7 @@ def contains_unpublished_changes(
             "publishable_entity__draft",
             "publishable_entity__draft__version",
             "publishable_entity__draft__version__containerentityversion__defined_list",
-        ).get(pk=container.container_entity)
+        ).get(pk=container.container_entity_id)
     else:
         pass # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit?
     assert isinstance(container, ContainerEntity)
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index c47c060f..f5460d1a 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -484,7 +484,7 @@ def test_query_count_of_contains_unpublished_changes(self):
         unit = self.create_unit_with_components(components)
         authoring_api.publish_all_drafts(self.learning_package.id)
         unit.refresh_from_db()
-        with self.assertNumQueries(3):
+        with self.assertNumQueries(2):
             assert authoring_api.contains_unpublished_changes(unit) is False
 
         # Modify the most recently created component:

From f9d7f486f85d43c17ea6b8f1ea86c4ffa39b6747 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 19 Feb 2025 16:47:51 -0800
Subject: [PATCH 20/27] fix: mypy typing

---
 .../apps/authoring/components/models.py       | 16 ++++-----
 .../apps/authoring/containers/api.py          | 16 +++++----
 .../apps/authoring/containers/models_mixin.py | 31 ++++------------
 .../apps/authoring/publishing/model_mixins.py | 36 +++++++------------
 openedx_learning/apps/authoring/units/api.py  | 14 ++++----
 openedx_learning/lib/managers.py              |  7 +++-
 6 files changed, 49 insertions(+), 71 deletions(-)

diff --git a/openedx_learning/apps/authoring/components/models.py b/openedx_learning/apps/authoring/components/models.py
index b0225fc9..f3ef8b03 100644
--- a/openedx_learning/apps/authoring/components/models.py
+++ b/openedx_learning/apps/authoring/components/models.py
@@ -17,6 +17,8 @@
 """
 from __future__ import annotations
 
+from typing import ClassVar
+
 from django.db import models
 
 from ....lib.fields import case_sensitive_char_field, immutable_uuid_field, key_field
@@ -76,7 +78,7 @@ def __str__(self):
         return f"{self.namespace}:{self.name}"
 
 
-class Component(PublishableEntityMixin):  # type: ignore[django-manager-missing]
+class Component(PublishableEntityMixin):
     """
     This represents any Component that has ever existed in a LearningPackage.
 
@@ -120,14 +122,12 @@ class Component(PublishableEntityMixin):  # type: ignore[django-manager-missing]
     Make a foreign key to the Component model when you need a stable reference
     that will exist for as long as the LearningPackage itself exists.
     """
-    # Tell mypy what type our objects manager has.
-    # It's actually PublishableEntityMixinManager, but that has the exact same
-    # interface as the base manager class.
-    objects: models.Manager[Component] = WithRelationsManager(
+    # Set up our custom manager. It has the same API as the default one, but selects related objects by default.
+    objects: ClassVar[WithRelationsManager[Component]] = WithRelationsManager(  # type: ignore[assignment]
         'component_type'
     )
 
-    with_publishing_relations: models.Manager[Component] = WithRelationsManager(
+    with_publishing_relations = WithRelationsManager(
         'component_type',
         'publishable_entity',
         'publishable_entity__draft__version',
@@ -201,10 +201,6 @@ class ComponentVersion(PublishableEntityVersionMixin):
     This holds the content using a M:M relationship with Content via
     ComponentVersionContent.
     """
-    # Tell mypy what type our objects manager has.
-    # It's actually PublishableEntityVersionMixinManager, but that has the exact
-    # same interface as the base manager class.
-    objects: models.Manager[ComponentVersion]
 
     # This is technically redundant, since we can get this through
     # publishable_entity_version.publishable.component, but this is more
diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index f8c37944..d6a76a86 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -82,7 +82,7 @@ def create_next_defined_list(
     new_entity_list: EntityList,
     entity_pks: list[int],
     entity_version_pks: list[int | None],
-) -> EntityListRow:
+) -> EntityList:
     """
     [ 🛑 UNSTABLE ]
     Create new entity list rows for an entity list.
@@ -384,7 +384,7 @@ def create_container_and_version(
     title: str,
     publishable_entities_pks: list[int],
     entity_version_pks: list[int | None],
-) -> ContainerEntityVersion:
+) -> tuple[ContainerEntity, ContainerEntityVersion]:
     """
     [ 🛑 UNSTABLE ]
     Create a new container and its first version.
@@ -490,10 +490,12 @@ def get_entities_in_published_container(
     # different?
     entity_list = []
     for row in cev.defined_list.entitylistrow_set.order_by("order_num"):
-        entity_list.append(ContainerEntityListEntry(
-            entity_version=row.entity_version or row.entity.published.version,
-            pinned=row.entity_version is not None,
-        ))
+        entity_version = row.entity_version or row.entity.published.version
+        if entity_version is not None:  # As long as this hasn't been soft-deleted:
+            entity_list.append(ContainerEntityListEntry(
+                entity_version=entity_version,
+                pinned=row.entity_version is not None,
+            ))
     return entity_list
 
 
@@ -536,7 +538,7 @@ def contains_unpublished_changes(
     ):
         try:
             child_container = row.entity.containerentity
-        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:  # pylint: disable=no-member
+        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:  # type: ignore[attr-defined] # pylint: disable=no-member
             child_container = None
         if child_container:
             child_container = row.entity.containerentity
diff --git a/openedx_learning/apps/authoring/containers/models_mixin.py b/openedx_learning/apps/authoring/containers/models_mixin.py
index 90477566..d7402570 100644
--- a/openedx_learning/apps/authoring/containers/models_mixin.py
+++ b/openedx_learning/apps/authoring/containers/models_mixin.py
@@ -2,9 +2,9 @@
 Mixins for models that implement containers
 """
 from __future__ import annotations
+from typing import ClassVar, Self
 
 from django.db import models
-from django.db.models.query import QuerySet
 
 from openedx_learning.apps.authoring.containers.models import (
     ContainerEntity,
@@ -14,6 +14,7 @@
     PublishableEntityMixin,
     PublishableEntityVersionMixin,
 )
+from openedx_learning.lib.managers import WithRelationsManager
 
 __all__ = [
     "ContainerEntityMixin",
@@ -30,17 +31,8 @@ class ContainerEntityMixin(PublishableEntityMixin):
     If you use this class, you *MUST* also use ContainerEntityVersionMixin
     """
 
-    class ContainerEntityMixinManager(models.Manager):
-        def get_queryset(self) -> QuerySet:
-            return (
-                super()
-                .get_queryset()
-                .select_related(
-                    "container_entity",
-                )
-            )
-
-    objects: models.Manager[ContainerEntityMixin] = ContainerEntityMixinManager()
+    # select these related entities by default for all queries
+    objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager("container_entity")  # type: ignore[assignment]
 
     container_entity = models.OneToOneField(
         ContainerEntity,
@@ -68,18 +60,9 @@ class ContainerEntityVersionMixin(PublishableEntityVersionMixin):
     If you use this class, you *MUST* also use ContainerEntityMixin
     """
 
-    class ContainerEntityVersionMixinManager(models.Manager):
-        def get_queryset(self) -> QuerySet:
-            return (
-                super()
-                .get_queryset()
-                .select_related(
-                    "container_entity_version",
-                )
-            )
-
-    objects: models.Manager[ContainerEntityVersionMixin] = (
-        ContainerEntityVersionMixinManager()
+    # select these related entities by default for all queries
+    objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager(  # type: ignore[assignment]
+        "container_entity_version",
     )
 
     container_entity_version = models.OneToOneField(
diff --git a/openedx_learning/apps/authoring/publishing/model_mixins.py b/openedx_learning/apps/authoring/publishing/model_mixins.py
index 85ef6c96..8743b333 100644
--- a/openedx_learning/apps/authoring/publishing/model_mixins.py
+++ b/openedx_learning/apps/authoring/publishing/model_mixins.py
@@ -4,10 +4,12 @@
 from __future__ import annotations
 
 from functools import cached_property
+from typing import ClassVar, Self
 
 from django.core.exceptions import ImproperlyConfigured
 from django.db import models
-from django.db.models.query import QuerySet
+
+from openedx_learning.lib.managers import WithRelationsManager
 
 from .models import PublishableEntity, PublishableEntityVersion
 
@@ -28,17 +30,12 @@ class PublishableEntityMixin(models.Model):
     the publishing app's api.register_content_models (see its docstring for
     details).
     """
-
-    class PublishableEntityMixinManager(models.Manager):
-        def get_queryset(self) -> QuerySet:
-            return super().get_queryset() \
-                          .select_related(
-                              "publishable_entity",
-                              "publishable_entity__published",
-                              "publishable_entity__draft",
-                          )
-
-    objects: models.Manager[PublishableEntityMixin] = PublishableEntityMixinManager()
+    # select these related entities by default for all queries
+    objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager(
+        "publishable_entity",
+        "publishable_entity__published",
+        "publishable_entity__draft",
+    )
 
     publishable_entity = models.OneToOneField(
         PublishableEntity, on_delete=models.CASCADE, primary_key=True
@@ -294,17 +291,10 @@ class PublishableEntityVersionMixin(models.Model):
     details).
     """
 
-    class PublishableEntityVersionMixinManager(models.Manager):
-        def get_queryset(self) -> QuerySet:
-            return (
-                super()
-                .get_queryset()
-                .select_related(
-                    "publishable_entity_version",
-                )
-            )
-
-    objects: models.Manager[PublishableEntityVersionMixin] = PublishableEntityVersionMixinManager()
+    # select these related entities by default for all queries
+    objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager(
+        "publishable_entity_version",
+    )
 
     publishable_entity_version = models.OneToOneField(
         PublishableEntityVersion, on_delete=models.CASCADE, primary_key=True
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 9d484f0b..74cfca17 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -62,7 +62,7 @@ def create_unit_version(
     entity_version_pks: list[int | None],
     created: datetime,
     created_by: int | None = None,
-) -> Unit:
+) -> UnitVersion:
     """
     [ 🛑 UNSTABLE ] Create a new unit version.
 
@@ -100,7 +100,7 @@ def create_next_unit_version(
     components: list[Component | ComponentVersion],
     created: datetime,
     created_by: int | None = None,
-) -> Unit:
+) -> UnitVersion:
     """
     [ 🛑 UNSTABLE ] Create the next unit version.
 
@@ -237,11 +237,13 @@ def get_components_in_draft_unit(
 
 def get_components_in_published_unit(
     unit: Unit,
-) -> list[UnitListEntry]:
+) -> list[UnitListEntry] | None:
     """
     [ 🛑 UNSTABLE ]
     Get the list of entities and their versions in the published version of the
     given container.
+
+    Returns None if the unit was never published (TODO: should it throw instead?).
     """
     assert isinstance(unit, Unit)
     published_entities = container_api.get_entities_in_published_container(unit)
@@ -275,10 +277,10 @@ def get_components_in_published_unit_as_of(
           ancestors of every modified PublishableEntity in the publish.
     """
     assert isinstance(unit, Unit)
-    unit_pub_entity_version = get_published_version_as_of(unit.publishable_entity, publish_log_id)
+    unit_pub_entity_version = get_published_version_as_of(unit.publishable_entity_id, publish_log_id)
     if unit_pub_entity_version is None:
         return None  # This unit was not published as of the given PublishLog ID.
-    unit_version = unit_pub_entity_version.unitversion
+    unit_version = unit_pub_entity_version.unitversion  # type: ignore[attr-defined]
 
     entity_list = []
     rows = unit_version.container_entity_version.defined_list.entitylistrow_set.order_by("order_num")
@@ -290,7 +292,7 @@ def get_components_in_published_unit_as_of(
         else:
             # Unpinned component - figure out what its latest published version was.
             # This is not optimized. It could be done in one query per unit rather than one query per component.
-            pub_entity_version = get_published_version_as_of(row.entity, publish_log_id)
+            pub_entity_version = get_published_version_as_of(row.entity_id, publish_log_id)
             if pub_entity_version:
                 entity_list.append(UnitListEntry(component_version=pub_entity_version.componentversion, pinned=False))
     return entity_list
diff --git a/openedx_learning/lib/managers.py b/openedx_learning/lib/managers.py
index b21f0a6b..cc8395a9 100644
--- a/openedx_learning/lib/managers.py
+++ b/openedx_learning/lib/managers.py
@@ -1,11 +1,16 @@
 """
 Custom Django ORM Managers.
 """
+from __future__ import annotations
+from typing import TypeVar
+
 from django.db import models
 from django.db.models.query import QuerySet
 
+M = TypeVar('M', bound=models.Model)
+
 
-class WithRelationsManager(models.Manager):
+class WithRelationsManager(models.Manager[M]):
     """
     Custom Manager that adds select_related to the default queryset.
 

From 703c0cf008519600fb98addda17ef636717b4070 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 20 Feb 2025 15:00:14 -0800
Subject: [PATCH 21/27] fix: verify that all container entities are in the
 right learning package

---
 openedx_learning/apps/authoring/containers/api.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index d6a76a86..4ac55036 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -7,6 +7,7 @@
 from dataclasses import dataclass
 from datetime import datetime
 
+from django.core.exceptions import ValidationError
 from django.db.transaction import atomic
 from django.db.models import QuerySet
 
@@ -315,6 +316,13 @@ def create_next_container_version(
     Returns:
         The newly created container version.
     """
+    # Do a quick check that the given entities are in the right learning package:
+    if PublishableEntity.objects.filter(
+        pk__in=publishable_entities_pks,
+    ).exclude(
+        learning_package_id=entity.learning_package_id,
+    ).exists():
+        raise ValidationError("Container entities must be from the same learning package.")
     with atomic():
         container = ContainerEntity.objects.get(pk=container_pk)
         last_version = container.versioning.latest

From f31c1de22e485aa1115aed39f36b8c0970ef3013 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 20 Feb 2025 15:05:47 -0800
Subject: [PATCH 22/27] chore: fix quality issues

---
 openedx_learning/api/authoring.py              |  2 +-
 openedx_learning/api/authoring_models.py       |  2 +-
 .../apps/authoring/containers/api.py           | 18 +++++++-----------
 .../apps/authoring/containers/apps.py          |  1 -
 .../containers/migrations/0001_initial.py      |  2 +-
 .../apps/authoring/containers/models.py        | 11 +++--------
 .../apps/authoring/containers/models_mixin.py  |  6 ++----
 openedx_learning/apps/authoring/units/api.py   |  4 ++--
 .../authoring/units/migrations/0001_initial.py |  2 +-
 openedx_learning/lib/managers.py               |  1 +
 .../apps/authoring/units/test_api.py           |  2 +-
 .../openedx_tagging/core/tagging/test_views.py | 10 +++++++++-
 12 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/openedx_learning/api/authoring.py b/openedx_learning/api/authoring.py
index 2fe11e93..de756616 100644
--- a/openedx_learning/api/authoring.py
+++ b/openedx_learning/api/authoring.py
@@ -11,9 +11,9 @@
 # pylint: disable=wildcard-import
 from ..apps.authoring.collections.api import *
 from ..apps.authoring.components.api import *
+from ..apps.authoring.containers.api import *
 from ..apps.authoring.contents.api import *
 from ..apps.authoring.publishing.api import *
-from ..apps.authoring.containers.api import *
 from ..apps.authoring.units.api import *
 
 # This was renamed after the authoring API refactoring pushed this and other
diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py
index f8653b2d..8c7752b2 100644
--- a/openedx_learning/api/authoring_models.py
+++ b/openedx_learning/api/authoring_models.py
@@ -9,8 +9,8 @@
 # pylint: disable=wildcard-import
 from ..apps.authoring.collections.models import *
 from ..apps.authoring.components.models import *
+from ..apps.authoring.containers.models import *
 from ..apps.authoring.contents.models import *
 from ..apps.authoring.publishing.model_mixins import *
 from ..apps.authoring.publishing.models import *
-from ..apps.authoring.containers.models import *
 from ..apps.authoring.units.models import *
diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 4ac55036..39231154 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -8,19 +8,14 @@
 from datetime import datetime
 
 from django.core.exceptions import ValidationError
-from django.db.transaction import atomic
 from django.db.models import QuerySet
+from django.db.transaction import atomic
 
 from openedx_learning.apps.authoring.containers.models_mixin import ContainerEntityMixin
-from ..containers.models import (
-    ContainerEntity,
-    ContainerEntityVersion,
-    EntityList,
-    EntityListRow,
-)
-from ..publishing.models import PublishableEntity, PublishableEntityVersion
-from ..publishing import api as publishing_api
 
+from ..containers.models import ContainerEntity, ContainerEntityVersion, EntityList, EntityListRow
+from ..publishing import api as publishing_api
+from ..publishing.models import PublishableEntity, PublishableEntityVersion
 
 # 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured
 #              out our approach to dynamic content (randomized, A/B tests, etc.)
@@ -124,6 +119,7 @@ def create_next_defined_list(
         EntityListRow.objects.bulk_create(new_rows)
     return new_entity_list
 
+
 def create_defined_list_with_rows(
     entity_pks: list[int],
     entity_version_pks: list[int | None],
@@ -527,7 +523,7 @@ def contains_unpublished_changes(
             "publishable_entity__draft__version__containerentityversion__defined_list",
         ).get(pk=container.container_entity_id)
     else:
-        pass # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit?
+        pass  # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit?
     assert isinstance(container, ContainerEntity)
 
     if container.versioning.has_unpublished_changes:
@@ -546,7 +542,7 @@ def contains_unpublished_changes(
     ):
         try:
             child_container = row.entity.containerentity
-        except PublishableEntity.containerentity.RelatedObjectDoesNotExist:  # type: ignore[attr-defined] # pylint: disable=no-member
+        except ContainerEntity.DoesNotExist:
             child_container = None
         if child_container:
             child_container = row.entity.containerentity
diff --git a/openedx_learning/apps/authoring/containers/apps.py b/openedx_learning/apps/authoring/containers/apps.py
index e8b2e36a..95850cac 100644
--- a/openedx_learning/apps/authoring/containers/apps.py
+++ b/openedx_learning/apps/authoring/containers/apps.py
@@ -15,7 +15,6 @@ class ContainersConfig(AppConfig):
     default_auto_field = "django.db.models.BigAutoField"
     label = "oel_containers"
 
-
     def ready(self):
         """
         Register ContainerEntity and ContainerEntityVersion.
diff --git a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
index 26583c98..be274d59 100644
--- a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
+++ b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
@@ -1,7 +1,7 @@
 # Generated by Django 4.2.19 on 2025-02-14 23:04
 
-from django.db import migrations, models
 import django.db.models.deletion
+from django.db import migrations, models
 
 
 class Migration(migrations.Migration):
diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py
index 8ae50936..6de71a21 100644
--- a/openedx_learning/apps/authoring/containers/models.py
+++ b/openedx_learning/apps/authoring/containers/models.py
@@ -3,14 +3,9 @@
 """
 from django.db import models
 
-from openedx_learning.apps.authoring.publishing.models import (
-    PublishableEntity,
-    PublishableEntityVersion,
-)
-from ..publishing.model_mixins import (
-    PublishableEntityMixin,
-    PublishableEntityVersionMixin,
-)
+from openedx_learning.apps.authoring.publishing.models import PublishableEntity, PublishableEntityVersion
+
+from ..publishing.model_mixins import PublishableEntityMixin, PublishableEntityVersionMixin
 
 __all__ = [
     "ContainerEntity",
diff --git a/openedx_learning/apps/authoring/containers/models_mixin.py b/openedx_learning/apps/authoring/containers/models_mixin.py
index d7402570..99ea8cc2 100644
--- a/openedx_learning/apps/authoring/containers/models_mixin.py
+++ b/openedx_learning/apps/authoring/containers/models_mixin.py
@@ -2,14 +2,12 @@
 Mixins for models that implement containers
 """
 from __future__ import annotations
+
 from typing import ClassVar, Self
 
 from django.db import models
 
-from openedx_learning.apps.authoring.containers.models import (
-    ContainerEntity,
-    ContainerEntityVersion,
-)
+from openedx_learning.apps.authoring.containers.models import ContainerEntity, ContainerEntityVersion
 from openedx_learning.apps.authoring.publishing.model_mixins import (
     PublishableEntityMixin,
     PublishableEntityVersionMixin,
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 74cfca17..8b3f0448 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -8,11 +8,11 @@
 from django.db.transaction import atomic
 
 from openedx_learning.apps.authoring.components.models import Component, ComponentVersion
-from ..publishing.api import get_published_version_as_of
+
 from ..containers import api as container_api
+from ..publishing.api import get_published_version_as_of
 from .models import Unit, UnitVersion
 
-
 # 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured
 #              out our approach to dynamic content (randomized, A/B tests, etc.)
 __all__ = [
diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py
index 3e3171c8..537264ee 100644
--- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py
+++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py
@@ -1,7 +1,7 @@
 # Generated by Django 4.2.16 on 2024-10-30 11:36
 
-from django.db import migrations, models
 import django.db.models.deletion
+from django.db import migrations, models
 
 
 class Migration(migrations.Migration):
diff --git a/openedx_learning/lib/managers.py b/openedx_learning/lib/managers.py
index cc8395a9..0298c87b 100644
--- a/openedx_learning/lib/managers.py
+++ b/openedx_learning/lib/managers.py
@@ -2,6 +2,7 @@
 Custom Django ORM Managers.
 """
 from __future__ import annotations
+
 from typing import TypeVar
 
 from django.db import models
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index f5460d1a..48852c30 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -2,11 +2,11 @@
 Basic tests for the units API.
 """
 import pytest
-
 from django.core.exceptions import ValidationError
 
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
+
 from ..components.test_api import ComponentTestCase
 
 Entry = authoring_api.UnitListEntry
diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py
index 7b24a1f6..181a5509 100644
--- a/tests/openedx_tagging/core/tagging/test_views.py
+++ b/tests/openedx_tagging/core/tagging/test_views.py
@@ -1042,7 +1042,15 @@ def test_object_tags_remaining_http_methods(
         ("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"),
     )
     @ddt.unpack
-    def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status, object_id):  # pylint: disable=too-many-positional-arguments
+    def test_tag_object(  # pylint: disable=too-many-positional-arguments
+        self,
+        user_attr,
+        taxonomy_attr,
+        taxonomy_flags,
+        tag_values,
+        expected_status,
+        object_id,
+    ):
         if user_attr:
             user = getattr(self, user_attr)
             self.client.force_authenticate(user=user)

From e07a741991f42b93cf152857d4e20a16b7e81eec Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 20 Feb 2025 15:46:53 -0800
Subject: [PATCH 23/27] test: tests related to [soft] deletion/removal of
 components

---
 .../apps/authoring/containers/api.py          |  12 +-
 .../apps/authoring/units/test_api.py          | 166 +++++++++++++++---
 2 files changed, 151 insertions(+), 27 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 39231154..7df5fe0e 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -463,10 +463,13 @@ def get_entities_in_draft_container(
     entity_list = []
     defined_list = container.versioning.draft.defined_list
     for row in defined_list.entitylistrow_set.order_by("order_num"):
-        entity_list.append(ContainerEntityListEntry(
-            entity_version=row.entity_version or row.entity.draft.version,
-            pinned=row.entity_version is not None,
-        ))
+        entity_version = row.entity_version or row.entity.draft.version
+        if entity_version is not None:  # As long as this hasn't been soft-deleted:
+            entity_list.append(ContainerEntityListEntry(
+                entity_version=row.entity_version or row.entity.draft.version,
+                pinned=row.entity_version is not None,
+            ))
+        # else should we indicate somehow a deleted item was here?
     return entity_list
 
 
@@ -500,6 +503,7 @@ def get_entities_in_published_container(
                 entity_version=entity_version,
                 pinned=row.entity_version is not None,
             ))
+        # else should we indicate somehow a deleted item was here?
     return entity_list
 
 
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 48852c30..d4f8af58 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -1,6 +1,7 @@
 """
 Basic tests for the units API.
 """
+import ddt  # type: ignore[import]
 import pytest
 from django.core.exceptions import ValidationError
 
@@ -12,24 +13,30 @@
 Entry = authoring_api.UnitListEntry
 
 
+@ddt.ddt
 class UnitTestCase(ComponentTestCase):
     """ Test cases for Units (containers of components) """
 
     def setUp(self) -> None:
         super().setUp()
-        self.component_1, self.component_1_v1 = authoring_api.create_component_and_version(
-            self.learning_package.id,
-            component_type=self.problem_type,
-            local_key="Query Counting",
+        self.component_1, self.component_1_v1 = self.create_component(
+            key="Query Counting",
             title="Querying Counting Problem",
-            created=self.now,
-            created_by=None,
         )
-        self.component_2, self.component_2_v1 = authoring_api.create_component_and_version(
+        self.component_2, self.component_2_v1 = self.create_component(
+            key="Query Counting (2)",
+            title="Querying Counting Problem (2)",
+        )
+
+    def create_component(self, *, title: str = "Test Component", key: str = "component:1") -> tuple[
+        authoring_models.Component, authoring_models.ComponentVersion
+    ]:
+        """ Helper method to quickly create a component """
+        return authoring_api.create_component_and_version(
             self.learning_package.id,
             component_type=self.problem_type,
-            local_key="Query Counting (2)",
-            title="Querying Counting Problem (2)",
+            local_key=key,
+            title=title,
             created=self.now,
             created_by=None,
         )
@@ -393,13 +400,9 @@ def test_publishing_shared_component(self):
         Everything is "unpinned".
         """
         # 1️⃣ Create the units and publish them:
-        (c1, c1_v1), (c2, _c2_v1), (c3, c3_v1), (c4, c4_v1), (c5, c5_v1) = [authoring_api.create_component_and_version(
-            self.learning_package.id,
-            component_type=self.problem_type,
-            local_key=f"C{i}",
-            title=f"Component {i}",
-            created=self.now,
-        ) for i in range(1, 6)]
+        (c1, c1_v1), (c2, _c2_v1), (c3, c3_v1), (c4, c4_v1), (c5, c5_v1) = [
+            self.create_component(key=f"C{i}", title=f"Component {i}") for i in range(1, 6)
+        ]
         unit1 = self.create_unit_with_components([c1, c2, c3], title="Unit 1", key="unit:1")
         unit2 = self.create_unit_with_components([c2, c4, c5], title="Unit 2", key="unit:2")
         authoring_api.publish_all_drafts(self.learning_package.id)
@@ -473,12 +476,9 @@ def test_query_count_of_contains_unpublished_changes(self):
         component_count = 100
         components = []
         for i in range(component_count):
-            component, _version = authoring_api.create_component_and_version(
-                self.learning_package.id,
-                component_type=self.problem_type,
-                local_key=f"Query Counting {i}",
+            component, _version = self.create_component(
+                key=f"Query Counting {i}",
                 title=f"Querying Counting Problem {i}",
-                created=self.now,
             )
             components.append(component)
         unit = self.create_unit_with_components(components)
@@ -492,12 +492,132 @@ def test_query_count_of_contains_unpublished_changes(self):
         with self.assertNumQueries(2):
             assert authoring_api.contains_unpublished_changes(unit) is True
 
+    @ddt.data(True, False)
+    @pytest.mark.skip(reason="FIXME: we don't yet prevent adding soft-deleted components to units")
+    def test_cannot_add_soft_deleted_component(self, publish_first):
+        """
+        Test that a soft-deleted component cannot be added to a unit.
+
+        Although it's valid for units to contain soft-deleted components (by
+        deleting the component after adding it), it is likely a mistake if
+        you're trying to add one to the unit.
+        """
+        component, _cv = self.create_component(title="Deleted component")
+        if publish_first:
+            # Publish the component:
+            authoring_api.publish_all_drafts(self.learning_package.id)
+        # Now delete it. The draft version is now deleted:
+        authoring_api.soft_delete_draft(component.pk)
+        # Now try adding that component to a unit:
+        with pytest.raises(ValidationError, match="component is deleted"):
+            self.create_unit_with_components([component])
+
+    def test_removing_component(self):
+        """ Test removing a component from a unit (but not deleting it) """
+        unit = self.create_unit_with_components([self.component_1, self.component_2])
+        authoring_api.publish_all_drafts(self.learning_package.id)
+
+        # Now remove component 2
+        authoring_api.create_next_unit_version(
+            unit=unit,
+            title="Revised with component 2 deleted",
+            components=[self.component_1],  # component 2 is gone
+            created=self.now,
+        )
+
+        # Now it should not be listed in the unit:
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            Entry(self.component_1_v1),
+        ]
+        unit.refresh_from_db()
+        assert unit.versioning.has_unpublished_changes  # The unit itself and its component list have change
+        assert authoring_api.contains_unpublished_changes(unit)
+        # The published version of the unit is not yet affected:
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+            Entry(self.component_2_v1),
+        ]
+
+        # But when we publish the new unit version with the removal, the published version is affected:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        # FIXME: Refreshing the unit is necessary here because get_entities_in_published_container() accesses
+        # container_entity.versioning.published, and .versioning is cached with the old version. But this seems like
+        # a footgun?
+        unit.refresh_from_db()
+        assert authoring_api.contains_unpublished_changes(unit) is False
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+        ]
+
+    def test_soft_deleting_component(self):
+        """ Test soft deleting a component that's in a unit (but not removing it) """
+        unit = self.create_unit_with_components([self.component_1, self.component_2])
+        authoring_api.publish_all_drafts(self.learning_package.id)
+
+        # Now soft delete component 2
+        authoring_api.soft_delete_draft(self.component_2.pk)
+
+        # Now it should not be listed in the unit:
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            Entry(self.component_1_v1),
+            # component 2 is soft deleted from the draft.
+            # TODO: should we return some kind of placeholder here, to indicate that a component is still listed in the
+            # unit's component list but has been soft deleted, and will be fully deleted when published, or restored if
+            # reverted?
+        ]
+        assert unit.versioning.has_unpublished_changes is False  # The unit itself and its component list is not changed
+        assert authoring_api.contains_unpublished_changes(unit)  # But it CONTAINS an unpublished change (a deletion)
+        # The published version of the unit is not yet affected:
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+            Entry(self.component_2_v1),
+        ]
+
+        # But when we publish the deletion, the published version is affected:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        assert authoring_api.contains_unpublished_changes(unit) is False
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+        ]
+
+    def test_soft_deleting_and_removing_component(self):
+        """ Test soft deleting a component that's in a unit AND removing it """
+        unit = self.create_unit_with_components([self.component_1, self.component_2])
+        authoring_api.publish_all_drafts(self.learning_package.id)
+
+        # Now soft delete component 2
+        authoring_api.soft_delete_draft(self.component_2.pk)
+        # And remove it from the unit:
+        authoring_api.create_next_unit_version(
+            unit=unit,
+            title="Revised with component 2 deleted",
+            components=[self.component_1],
+            created=self.now,
+        )
+
+        # Now it should not be listed in the unit:
+        assert authoring_api.get_components_in_draft_unit(unit) == [
+            Entry(self.component_1_v1),
+        ]
+        assert unit.versioning.has_unpublished_changes is True
+        assert authoring_api.contains_unpublished_changes(unit)
+        # The published version of the unit is not yet affected:
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+            Entry(self.component_2_v1),
+        ]
+
+        # But when we publish the deletion, the published version is affected:
+        authoring_api.publish_all_drafts(self.learning_package.id)
+        assert authoring_api.contains_unpublished_changes(unit) is False
+        assert authoring_api.get_components_in_published_unit(unit) == [
+            Entry(self.component_1_v1),
+        ]
+
     # Test the query counts of various operations
-    # Test that soft-deleting a component doesn't show the unit as changed but does show it contains changes ?
     # Test that only components can be added to units
     # Test that components must be in the same learning package
     # Test that invalid component PKs cannot be added to a unit
-    # Test that soft-deleted components cannot be added to a unit?
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks
     # Test that publishing a unit publishes its child components automatically
     # Test that publishing a component does NOT publish changes to its parent unit

From e09a20e4bc05e6ec017a6d76353aeae0a3bd9c29 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 20 Feb 2025 16:46:15 -0800
Subject: [PATCH 24/27] refactor: consolidate
 defined_list/initial_list/frozen_list into entity_list

---
 .../apps/authoring/containers/api.py          | 161 ++----------------
 .../containers/migrations/0001_initial.py     |   4 +-
 .../apps/authoring/containers/models.py       |  46 +----
 openedx_learning/apps/authoring/units/api.py  |   2 +-
 .../apps/authoring/units/test_api.py          |  64 +------
 5 files changed, 22 insertions(+), 255 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 7df5fe0e..0565dd04 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -73,9 +73,7 @@ def create_entity_list() -> EntityList:
     return EntityList.objects.create()
 
 
-def create_next_defined_list(
-    previous_entity_list: EntityList | None,  # pylint: disable=unused-argument
-    new_entity_list: EntityList,
+def create_entity_list_with_rows(
     entity_pks: list[int],
     entity_version_pks: list[int | None],
 ) -> EntityList:
@@ -84,52 +82,6 @@ def create_next_defined_list(
     Create new entity list rows for an entity list.
 
     Args:
-        previous_entity_list: The previous entity list that the new entity list is based on.
-        new_entity_list: The entity list to create the rows for.
-        entity_pks: The IDs of the publishable entities that the entity list rows reference.
-        entity_version_pks: The IDs of the draft versions of the entities
-            (PublishableEntityVersion) that the entity list rows reference, or
-            Nones for "unpinned" (default).
-
-    Returns:
-        The newly created entity list rows.
-    """
-    order_nums = range(len(entity_pks))
-    with atomic():
-        # Case 1: create first container version (no previous rows created for container)
-        # 1. Create new rows for the entity list
-        # Case 2: create next container version (previous rows created for container)
-        # 1. Get all the rows in the previous version entity list
-        # 2. Only associate existent rows to the new entity list iff: the order is the same, the PublishableEntity is in
-        #    entity_pks and versions are not pinned
-        # 3. If the order is different for a row with the PublishableEntity, create new row with the same
-        #    PublishableEntity for the new order and associate the new row to the new entity list
-        new_rows = []
-        for order_num, entity_pk, entity_version_pk in zip(
-            order_nums, entity_pks, entity_version_pks
-        ):
-            new_rows.append(
-                EntityListRow(
-                    entity_list=new_entity_list,
-                    entity_id=entity_pk,
-                    order_num=order_num,
-                    entity_version_id=entity_version_pk,
-                )
-            )
-        EntityListRow.objects.bulk_create(new_rows)
-    return new_entity_list
-
-
-def create_defined_list_with_rows(
-    entity_pks: list[int],
-    entity_version_pks: list[int | None],
-) -> EntityList:
-    """
-    [ 🛑 UNSTABLE ]
-    Create new entity list rows for an entity list.
-
-    Args:
-        entity_list: The entity list to create the rows for.
         entity_pks: The IDs of the publishable entities that the entity list rows reference.
         entity_version_pks: The IDs of the versions of the entities
             (PublishableEntityVersion) that the entity list rows reference, or
@@ -188,46 +140,6 @@ def get_entity_list_with_pinned_versions(
     return entity_list
 
 
-def check_unpinned_versions_in_defined_list(
-    defined_list: EntityList,
-) -> bool:
-    """
-    [ 🛑 UNSTABLE ]
-    Check if there are any unpinned versions in the defined list.
-
-    Args:
-        defined_list: The defined list to check for unpinned versions.
-
-    Returns:
-        True if there are unpinned versions in the defined list, False otherwise.
-    """
-    # Is there a way to short-circuit this?
-    return any(
-        row.entity_version is None
-        for row in defined_list.entitylistrow_set.all()
-    )
-
-
-def check_new_changes_in_defined_list(
-    entity_list: EntityList,  # pylint: disable=unused-argument
-    publishable_entities_pks: list[int],  # pylint: disable=unused-argument
-) -> bool:
-    """
-    [ 🛑 UNSTABLE ]
-    Check if there are any new changes in the defined list.
-
-    Args:
-        entity_list: The entity list to check for new changes.
-        publishable_entities: The publishable entities to check for new changes.
-
-    Returns:
-        True if there are new changes in the defined list, False otherwise.
-    """
-    # Is there a way to short-circuit this? Using queryset operations
-    # For simplicity, return True
-    return True
-
-
 def create_container_version(
     container_pk: int,
     version_num: int,
@@ -263,19 +175,14 @@ def create_container_version(
             created=created,
             created_by=created_by,
         )
-        defined_list = create_defined_list_with_rows(
+        entity_list = create_entity_list_with_rows(
             entity_pks=publishable_entities_pks,
             entity_version_pks=entity_version_pks,
         )
         container_version = ContainerEntityVersion.objects.create(
             publishable_entity_version=publishable_entity_version,
             container_id=container_pk,
-            defined_list=defined_list,
-            initial_list=defined_list,
-            # TODO: Check for unpinned versions in defined_list to know whether to point this to the defined_list
-            # point to None.
-            # If this is the first version ever created for this ContainerEntity, then start as None.
-            frozen_list=None,
+            entity_list=entity_list,
         )
     return container_version
 
@@ -330,50 +237,14 @@ def create_next_container_version(
             created=created,
             created_by=created_by,
         )
-        # 1. Check if there are any changes in the container's members
-        # 2. Pin versions in previous frozen list for last container version
-        # 3. Create new defined list for author changes
-        # 4. Pin versions in defined list to create initial list
-        # 5. Check for unpinned references in defined_list to determine if frozen_list should be None
-        # 6. Point frozen_list to None or defined_list
-        if check_new_changes_in_defined_list(
-            entity_list=last_version.defined_list,
-            publishable_entities_pks=publishable_entities_pks,
-        ):
-            # Only change if there are unpin versions in defined list, meaning last frozen list is None
-            # When does this has to happen? Before?
-            if not last_version.frozen_list:
-                last_version.frozen_list = get_entity_list_with_pinned_versions(
-                    rows=last_version.defined_list.entitylistrow_set.all()
-                )
-                last_version.save()
-            next_defined_list = create_next_defined_list(
-                previous_entity_list=last_version.defined_list,
-                new_entity_list=create_entity_list(),
-                entity_pks=publishable_entities_pks,
-                entity_version_pks=entity_version_pks,
-            )
-            next_initial_list = get_entity_list_with_pinned_versions(
-                rows=next_defined_list.entitylistrow_set.all()
-            )
-            if check_unpinned_versions_in_defined_list(next_defined_list):
-                next_frozen_list = None
-            else:
-                next_frozen_list = next_initial_list
-        else:
-            # Do I need to create new EntityList and copy rows?
-            # I do think so because frozen can change when creating a new version
-            # Does it need to change though?
-            # What would happen if I only change the title?
-            next_defined_list = last_version.defined_list
-            next_initial_list = last_version.initial_list
-            next_frozen_list = last_version.frozen_list
+        next_entity_list = create_entity_list_with_rows(
+            entity_pks=publishable_entities_pks,
+            entity_version_pks=entity_version_pks,
+        )
         next_container_version = ContainerEntityVersion.objects.create(
             publishable_entity_version=publishable_entity_version,
             container_id=container_pk,
-            defined_list=next_defined_list,
-            initial_list=next_initial_list,
-            frozen_list=next_frozen_list,
+            entity_list=next_entity_list,
         )
 
     return next_container_version
@@ -461,8 +332,7 @@ def get_entities_in_draft_container(
         container = container.container_entity
     assert isinstance(container, ContainerEntity)
     entity_list = []
-    defined_list = container.versioning.draft.defined_list
-    for row in defined_list.entitylistrow_set.order_by("order_num"):
+    for row in container.versioning.draft.entity_list.entitylistrow_set.order_by("order_num"):
         entity_version = row.entity_version or row.entity.draft.version
         if entity_version is not None:  # As long as this hasn't been soft-deleted:
             entity_list.append(ContainerEntityListEntry(
@@ -490,13 +360,8 @@ def get_entities_in_published_container(
     if cev is None:
         return None  # There is no published version of this container. Should this be an exception?
     assert isinstance(cev, ContainerEntityVersion)
-    # TODO: do we ever need frozen_list? e.g. when accessing a historical version?
-    # Doesn't make a lot of sense when the versions of the container don't capture many of the changes to the contents,
-    # e.g. container version 1 had component version 1 through 50, and container version 2 had component versions 51
-    # through 100, what is the point of tracking whether container version 1 "should" show v1 or v50 when they're wildly
-    # different?
     entity_list = []
-    for row in cev.defined_list.entitylistrow_set.order_by("order_num"):
+    for row in cev.entity_list.entitylistrow_set.order_by("order_num"):
         entity_version = row.entity_version or row.entity.published.version
         if entity_version is not None:  # As long as this hasn't been soft-deleted:
             entity_list.append(ContainerEntityListEntry(
@@ -524,7 +389,7 @@ def contains_unpublished_changes(
             "publishable_entity",
             "publishable_entity__draft",
             "publishable_entity__draft__version",
-            "publishable_entity__draft__version__containerentityversion__defined_list",
+            "publishable_entity__draft__version__containerentityversion__entity_list",
         ).get(pk=container.container_entity_id)
     else:
         pass  # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit?
@@ -534,12 +399,12 @@ def contains_unpublished_changes(
         return True
 
     # We only care about children that are un-pinned, since published changes to pinned children don't matter
-    defined_list = container.versioning.draft.defined_list
+    entity_list = container.versioning.draft.entity_list
 
     # TODO: This is a naive inefficient implementation but hopefully correct.
     # Once we know it's correct and have a good test suite, then we can optimize.
     # We will likely change to a tracking-based approach rather than a "scan for changes" based approach.
-    for row in defined_list.entitylistrow_set.filter(entity_version=None).select_related(
+    for row in entity_list.entitylistrow_set.filter(entity_version=None).select_related(
         "entity__containerentity",
         "entity__draft__version",
         "entity__published__version",
diff --git a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
index be274d59..ab1068e2 100644
--- a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
+++ b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py
@@ -43,9 +43,7 @@ class Migration(migrations.Migration):
             fields=[
                 ('publishable_entity_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentityversion')),
                 ('container', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_containers.containerentity')),
-                ('defined_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='defined_list', to='oel_containers.entitylist')),
-                ('frozen_list', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='frozen_list', to='oel_containers.entitylist')),
-                ('initial_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='initial_list', to='oel_containers.entitylist')),
+                ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='entity_list', to='oel_containers.entitylist')),
             ],
             options={
                 'abstract': False,
diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py
index 6de71a21..9d4ec990 100644
--- a/openedx_learning/apps/authoring/containers/models.py
+++ b/openedx_learning/apps/authoring/containers/models.py
@@ -21,7 +21,7 @@ class EntityList(models.Model):
     sometimes we'll want the same kind of data structure for things that we
     dynamically generate for individual students (e.g. Variants). EntityLists are
     anonymous in a sense–they're pointed to by ContainerEntityVersions and
-    other models, rather than being looked up by their own identifers.
+    other models, rather than being looked up by their own identifiers.
     """
 
 
@@ -92,7 +92,7 @@ class ContainerEntityVersion(PublishableEntityVersionMixin):
     The last looks a bit odd, but it's because *how we've defined the Unit* has
     changed if we decide to explicitly pin a set of versions for the children,
     and then later change our minds and move to a different set. It also just
-    makes things easier to reason about if we say that defined_list never
+    makes things easier to reason about if we say that entity_list never
     changes for a given ContainerEntityVersion.
     """
 
@@ -102,46 +102,10 @@ class ContainerEntityVersion(PublishableEntityVersionMixin):
         related_name="versions",
     )
 
-    # This is the EntityList that the author defines. This should never change,
-    # even if the things it references get soft-deleted (because we'll need to
-    # maintain it for reverts).
-    defined_list = models.ForeignKey(
+    # The list of entities (frozen and/or unfrozen) in this container
+    entity_list = models.ForeignKey(
         EntityList,
         on_delete=models.RESTRICT,
         null=False,
-        related_name="defined_list",
-    )
-
-    # inital_list is an EntityList where all the versions are pinned, to show
-    # what the exact versions of the children were at the time that the
-    # Container was created. We could technically derive this, but it would be
-    # awkward to query.
-    #
-    # If the Container was defined so that all references were pinned, then this
-    # can point to the exact same EntityList as defined_list.
-    initial_list = models.ForeignKey(
-        EntityList,
-        on_delete=models.RESTRICT,
-        null=False,
-        related_name="initial_list",
-    )
-
-    # This is the EntityList that's created when the next ContainerEntityVersion
-    # is created. All references in this list should be pinned, and it serves as
-    # "the last state the children were in for this version of the Container".
-    # If defined_list has only pinned references, this should point to the same
-    # EntityList as defined_list and initial_list.
-    #
-    # This value is mutable if and only if there are unpinned references in
-    # defined_list. In that case, frozen_list should start as None, and be
-    # updated to pin references when another version of this Container becomes
-    # the Draft version. But if this version ever becomes the Draft *again*
-    # (e.g. the user hits "discard changes" or some kind of revert happens),
-    # then we need to clear this back to None.
-    frozen_list = models.ForeignKey(
-        EntityList,
-        on_delete=models.RESTRICT,
-        null=True,
-        default=None,
-        related_name="frozen_list",
+        related_name="entity_list",
     )
diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py
index 8b3f0448..e6db8316 100644
--- a/openedx_learning/apps/authoring/units/api.py
+++ b/openedx_learning/apps/authoring/units/api.py
@@ -283,7 +283,7 @@ def get_components_in_published_unit_as_of(
     unit_version = unit_pub_entity_version.unitversion  # type: ignore[attr-defined]
 
     entity_list = []
-    rows = unit_version.container_entity_version.defined_list.entitylistrow_set.order_by("order_num")
+    rows = unit_version.container_entity_version.entity_list.entitylistrow_set.order_by("order_num")
     for row in rows:
         if row.entity_version is not None:
             component_version = row.entity_version.componentversion
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index d4f8af58..345f8b36 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -705,43 +705,7 @@ def test_next_version_with_different_different_title(self):
         2. The unit version number is 2.
         3. The unit version is in the unit's versions.
         4. The unit version's title is different from the previous version.
-        5. The user defined is the same as the previous version.
-        6. The frozen list is empty.
-        """
-
-    def test_check_author_defined_list_matches_components(self):
-        """Test checking the author defined list matches the components.
-
-        Expected results:
-        1. The author defined list matches the components used to create the unit version.
-        """
-
-    def test_check_initial_list_matches_components(self):
-        """Test checking the initial list matches the components.
-
-        Expected results:
-        1. The initial list matches the components (pinned) used to create the unit version.
-        """
-
-    def test_check_frozen_list_is_none_floating_versions(self):
-        """Test checking the frozen list is None when floating versions are used in the author defined list.
-
-        Expected results:
-        1. The frozen list is None.
-        """
-
-    def test_check_frozen_list_when_next_version_is_created(self):
-        """Test checking the frozen list when a new version is created.
-
-        Expected results:
-        1. The frozen list has pinned versions of the user defined list from the previous version.
-        """
-
-    def test_check_lists_equal_when_pinned_versions(self):
-        """Test checking the lists are equal when pinned versions are used.
-
-        Expected results:
-        1. The author defined list == initial list == frozen list.
+        5. The entity list is the same as the previous version.
         """
 
     def test_publish_unit_version(self):
@@ -768,29 +732,5 @@ def test_next_version_with_different_order(self):
         1. A new unit version is created.
         2. The unit version number is 2.
         3. The unit version is in the unit's versions.
-        4. The user defined list is different from the previous version.
-        5. The initial list contains the pinned versions of the defined list.
-        6. The frozen list is empty.
-        """
-
-    def test_soft_delete_component_from_units(self):
-        """Soft-delete a component from a unit.
-
-        Expected result:
-        After soft-deleting the component (draft), a new unit version (draft) is created for the unit.
-        """
-
-    def test_soft_delete_component_from_units_and_publish(self):
-        """Soft-delete a component from a unit and publish the unit.
-
-        Expected result:
-        After soft-deleting the component (draft), a new unit version (draft) is created for the unit.
-        Then, if the unit is published all units referencing the component are published as well.
-        """
-
-    def test_unit_version_becomes_draft_again(self):
-        """Test a unit version becomes a draft again.
-
-        Expected results:
-        1. The frozen list is None after the unit version becomes a draft again.
+        4. The entity list is different from the previous version.
         """

From 1fc456e864c1e03f29b1535dd3686c5e7b47d2fa Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 21 Feb 2025 15:53:04 -0800
Subject: [PATCH 25/27] test: add tests that publishes cascade down but not up

---
 .../apps/authoring/containers/api.py          |   2 +-
 .../apps/authoring/units/test_api.py          | 103 ++++++++++--------
 2 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py
index 0565dd04..1c31e5c2 100644
--- a/openedx_learning/apps/authoring/containers/api.py
+++ b/openedx_learning/apps/authoring/containers/api.py
@@ -421,7 +421,7 @@ def contains_unpublished_changes(
         else:
             # This is not a container:
             draft_pk = row.entity.draft.version_id if row.entity.draft else None
-            published_pk = row.entity.published.version_id if row.entity.published else None
+            published_pk = row.entity.published.version_id if hasattr(row.entity, "published") else None
             if draft_pk != published_pk:
                 return True
     return False
diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 345f8b36..26ff436c 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -220,6 +220,56 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self):
         ]
         assert authoring_api.get_components_in_published_unit(unit) is None
 
+    @pytest.mark.skip(reason="FIXME: auto-publishing children is not implemented yet")
+    def test_auto_publish_children(self):
+        """
+        Test that publishing a unit publishes its child components automatically.
+        """
+        # Create a draft unit with two draft components
+        unit = self.create_unit_with_components([self.component_1, self.component_2])
+        # Also create another component that's not in the unit at all:
+        other_component, _oc_v1 = self.create_component(title="A draft component not in the unit", key="component:3")
+
+        assert authoring_api.contains_unpublished_changes(unit)
+        assert self.component_1.versioning.published is None
+        assert self.component_2.versioning.published is None
+
+        # Publish ONLY the unit. This should however also auto-publish components 1 & 2 since they're children
+        authoring_api.publish_from_drafts(
+            self.learning_package.pk,
+            draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter(entity=unit.publishable_entity),
+        )
+        # Now all changes to the unit and to component 1 are published:
+        unit.refresh_from_db()
+        self.component_1.refresh_from_db()
+        assert unit.versioning.has_unpublished_changes is False  # Shallow check
+        assert self.component_1.versioning.has_unpublished_changes is False
+        assert authoring_api.contains_unpublished_changes(unit) is False  # Deep check
+        assert self.component_1.versioning.published == self.component_1_v1  # v1 is now the published version.
+
+        # But our other component that's outside the unit is not affected:
+        other_component.refresh_from_db()
+        assert other_component.versioning.has_unpublished_changes
+        assert other_component.versioning.published is None
+
+    def test_no_publish_parent(self):
+        """
+        Test that publishing a component does NOT publish changes to its parent unit
+        """
+        # Create a draft unit with two draft components
+        unit = self.create_unit_with_components([self.component_1, self.component_2])
+        assert unit.versioning.has_unpublished_changes
+        # Publish ONLY one of its child components
+        self.publish_component(self.component_1)
+        self.component_1.refresh_from_db()  # Clear cache on '.versioning'
+        assert self.component_1.versioning.has_unpublished_changes is False
+
+        # The unit that contains that component should still be unpublished:
+        unit.refresh_from_db()  # Clear cache on '.versioning'
+        assert unit.versioning.has_unpublished_changes
+        assert unit.versioning.published is None
+        assert authoring_api.get_components_in_published_unit(unit) is None
+
     def test_add_component_after_publish(self):
         """
         Adding a component to a published unit will create a new version and
@@ -340,11 +390,9 @@ def test_modify_pinned_component(self):
         assert authoring_api.get_components_in_published_unit(unit) == expected_unit_contents
 
     def test_create_two_units_with_same_components(self):
-        """Test creating two units with the same components.
-
-        Expected results:
-        1. Two different units are created.
-        2. The units have the same components.
+        """
+        Test creating two units with different combinations of the same two
+        components in each unit.
         """
         # Create a unit with component 2 unpinned, component 2 pinned 📌, and component 1:
         unit1 = self.create_unit_with_components([self.component_2, self.component_2_v1, self.component_1], key="u1")
@@ -542,7 +590,8 @@ def test_removing_component(self):
         authoring_api.publish_all_drafts(self.learning_package.id)
         # FIXME: Refreshing the unit is necessary here because get_entities_in_published_container() accesses
         # container_entity.versioning.published, and .versioning is cached with the old version. But this seems like
-        # a footgun?
+        # a footgun? We could avoid this if get_entities_in_published_container() took only an ID instead of an object,
+        # but that would involve additional database lookup(s).
         unit.refresh_from_db()
         assert authoring_api.contains_unpublished_changes(unit) is False
         assert authoring_api.get_components_in_published_unit(unit) == [
@@ -619,14 +668,14 @@ def test_soft_deleting_and_removing_component(self):
     # Test that components must be in the same learning package
     # Test that invalid component PKs cannot be added to a unit
     # Test that _version_pks=[] arguments must be related to publishable_entities_pks
-    # Test that publishing a unit publishes its child components automatically
-    # Test that publishing a component does NOT publish changes to its parent unit
     # Test that I can get a history of a given unit and all its children, including children that aren't currently in
     #     the unit and excluding children that are only in other units.
     # Test that I can get a history of a given unit and its children, that includes changes made to the child components
     #     while they were part of the unit but excludes changes made to those children while they were not part of
     #     the unit. 🫣
 
+    # TODO: test that I can find all the units that contain the given component.
+
     def test_snapshots_of_published_unit(self):
         """
         Test that we can access snapshots of the historic published version of
@@ -696,41 +745,3 @@ def test_snapshots_of_published_unit(self):
             "Component 1 as of checkpoint 3",  # we didn't modify these components so they're same as in snapshot 3
             "Component 2 as of checkpoint 3",  # we didn't modify these components so they're same as in snapshot 3
         ]
-
-    def test_next_version_with_different_different_title(self):
-        """Test creating a unit version with a different title.
-
-        Expected results:
-        1. A new unit version is created.
-        2. The unit version number is 2.
-        3. The unit version is in the unit's versions.
-        4. The unit version's title is different from the previous version.
-        5. The entity list is the same as the previous version.
-        """
-
-    def test_publish_unit_version(self):
-        """Test publish unpublished unit version.
-
-        Expected results:
-        1. The newly created unit version has unpublished changes.
-        2. The published version matches the unit version.
-        3. The draft version matches the unit version.
-        """
-
-    def test_publish_unit_with_unpublished_component(self):
-        """Test publishing a unit with an unpublished component.
-
-        Expected results:
-        1. The unit version is published.
-        2. The component is published.
-        """
-
-    def test_next_version_with_different_order(self):
-        """Test creating a unit version with different order of components.
-
-        Expected results:
-        1. A new unit version is created.
-        2. The unit version number is 2.
-        3. The unit version is in the unit's versions.
-        4. The entity list is different from the previous version.
-        """

From 722026d4fdb7360461d7ff637618d07a70211e9e Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 21 Feb 2025 15:57:30 -0800
Subject: [PATCH 26/27] test: update docs about further tests we should write

---
 .../openedx_learning/apps/authoring/units/test_api.py  | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index 26ff436c..f6b8a165 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -663,19 +663,19 @@ def test_soft_deleting_and_removing_component(self):
             Entry(self.component_1_v1),
         ]
 
+    # TODO: test that I can find all the units that contain the given component.
     # Test the query counts of various operations
-    # Test that only components can be added to units
-    # Test that components must be in the same learning package
     # Test that invalid component PKs cannot be added to a unit
-    # Test that _version_pks=[] arguments must be related to publishable_entities_pks
+    # Test that _version_pks=[] arguments must be related to publishable_entities_pks. Note: this is not actually
+    #     possible with the updated Units API I've implemented (where you only provide a list of Component or
+    #     ComponentVersion objects), but it is a potential problem with the low-level container APIs since they accept
+    #     separate lists of entity IDs vs. entity versions.
     # Test that I can get a history of a given unit and all its children, including children that aren't currently in
     #     the unit and excluding children that are only in other units.
     # Test that I can get a history of a given unit and its children, that includes changes made to the child components
     #     while they were part of the unit but excludes changes made to those children while they were not part of
     #     the unit. 🫣
 
-    # TODO: test that I can find all the units that contain the given component.
-
     def test_snapshots_of_published_unit(self):
         """
         Test that we can access snapshots of the historic published version of

From 97e17c741c219a6f804d120bf08ddcd0b73c987a Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Mon, 24 Feb 2025 15:28:46 -0800
Subject: [PATCH 27/27] test: add test for invalid component IDs

---
 .../apps/authoring/units/test_api.py          | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py
index f6b8a165..efe35290 100644
--- a/tests/openedx_learning/apps/authoring/units/test_api.py
+++ b/tests/openedx_learning/apps/authoring/units/test_api.py
@@ -4,6 +4,7 @@
 import ddt  # type: ignore[import]
 import pytest
 from django.core.exceptions import ValidationError
+from django.db import IntegrityError
 
 from openedx_learning.api import authoring as authoring_api
 from openedx_learning.api import authoring_models
@@ -140,6 +141,19 @@ def test_adding_external_components(self):
                 created_by=None,
             )
 
+    @ddt.data(True, False)
+    def test_cannot_add_invalid_ids(self, pin_version):
+        """
+        Test that non-existent components cannot be added to units
+        """
+        self.component_1.delete()
+        if pin_version:
+            components = [self.component_1_v1]
+        else:
+            components = [self.component_1]
+        with pytest.raises((IntegrityError, authoring_models.Component.DoesNotExist)):
+            self.create_unit_with_components(components)
+
     def test_create_empty_unit_and_version(self):
         """Test creating a unit with no components.
 
@@ -664,12 +678,6 @@ def test_soft_deleting_and_removing_component(self):
         ]
 
     # TODO: test that I can find all the units that contain the given component.
-    # Test the query counts of various operations
-    # Test that invalid component PKs cannot be added to a unit
-    # Test that _version_pks=[] arguments must be related to publishable_entities_pks. Note: this is not actually
-    #     possible with the updated Units API I've implemented (where you only provide a list of Component or
-    #     ComponentVersion objects), but it is a potential problem with the low-level container APIs since they accept
-    #     separate lists of entity IDs vs. entity versions.
     # Test that I can get a history of a given unit and all its children, including children that aren't currently in
     #     the unit and excluding children that are only in other units.
     # Test that I can get a history of a given unit and its children, that includes changes made to the child components