Encapsulate child entity + entity versions in Containers API [FC-0083] (#300)

This PR addresses in #278 (comment) by creating a dataclass that encapsulates a PublishableEntity.pk with an optional PublishableEntityVersion.pk, so that the container API methods don't have to pass in two separate lists when specifying pinned versions for a container's children.

This change only affects the (unstable) publishing app's container-related API. The (also unstable) Units API, which is used by edx-platform, remains unchanged.

Author: pomegranited

openedx_learning/__init__.py

@@ -2,4 +2,4 @@
 Open edX Learning ("Learning Core").
 """
 
-__version__ = "0.20.0"
+__version__ = "0.21.0"

openedx_learning/apps/authoring/publishing/api.py

@@ -76,6 +76,7 @@
     "get_containers",
     "ChildrenEntitiesAction",
     "ContainerEntityListEntry",
+    "ContainerEntityRow",
     "get_entities_in_container",
     "contains_unpublished_changes",
     "get_containers_with_entity",
@@ -639,8 +640,7 @@ def create_entity_list() -> EntityList:
 
 
 def create_entity_list_with_rows(
-    entity_pks: list[int],
-    entity_version_pks: list[int | None],
+    entity_rows: list[ContainerEntityRow],
     *,
     learning_package_id: int | None,
 ) -> EntityList:
@@ -649,10 +649,7 @@ def create_entity_list_with_rows(
     Create new entity list rows for an entity list.
 
     Args:
-        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
-            Nones for "unpinned" (default).
+        entity_rows: List of ContainerEntityRows specifying the publishable entity ID and version ID (if pinned).
         learning_package_id: Optional. Verify that all the entities are from
             the specified learning package.
 
@@ -662,27 +659,37 @@ def create_entity_list_with_rows(
     # Do a quick check that the given entities are in the right learning package:
     if learning_package_id:
         if PublishableEntity.objects.filter(
-            pk__in=entity_pks,
+            pk__in=[entity.entity_pk for entity in entity_rows],
         ).exclude(
             learning_package_id=learning_package_id,
         ).exists():
             raise ValidationError("Container entities must be from the same learning package.")
 
-    order_nums = range(len(entity_pks))
+    # Ensure that any pinned entity versions are linked to the correct entity
+    pinned_entities = {
+        entity.version_pk: entity.entity_pk
+        for entity in entity_rows if entity.pinned
+    }
+    if pinned_entities:
+        entity_versions = PublishableEntityVersion.objects.filter(
+            pk__in=pinned_entities.keys(),
+        ).only('pk', 'entity_id')
+        for entity_version in entity_versions:
+            if pinned_entities[entity_version.pk] != entity_version.entity_id:
+                raise ValidationError("Container entity versions must belong to the specified entity.")
+
     with atomic(savepoint=False):
 
         entity_list = create_entity_list()
         EntityListRow.objects.bulk_create(
             [
                 EntityListRow(
                     entity_list=entity_list,
-                    entity_id=entity_pk,
+                    entity_id=entity.entity_pk,
                     order_num=order_num,
-                    entity_version_id=entity_version_pk,
-                )
-                for order_num, entity_pk, entity_version_pk in zip(
-                    order_nums, entity_pks, entity_version_pks
+                    entity_version_id=entity.version_pk,
                 )
+                for order_num, entity in enumerate(entity_rows)
             ]
         )
     return entity_list
@@ -725,8 +732,7 @@ def create_container_version(
     version_num: int,
     *,
     title: str,
-    publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None] | None,
+    entity_rows: list[ContainerEntityRow],
     created: datetime,
     created_by: int | None,
     container_version_cls: type[ContainerVersionModel] = ContainerVersion,  # type: ignore[assignment]
@@ -739,8 +745,7 @@ def create_container_version(
         container_id: 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_pks: The IDs of the members of the container.
-        entity_version_pks: The IDs of the versions to pin to, if pinning is desired.
+        entity_rows: List of ContainerEntityRows specifying the publishable entity ID and version ID (if pinned).
         created: The date and time the container version was created.
         created_by: The ID of the user who created the container version.
         container_version_cls: The subclass of ContainerVersion to use, if applicable.
@@ -749,16 +754,13 @@ def create_container_version(
         The newly created container version.
     """
     assert title is not None
-    assert publishable_entities_pks is not None
+    assert entity_rows is not None
 
     with atomic(savepoint=False):
         container = Container.objects.select_related("publishable_entity").get(pk=container_id)
         entity = container.publishable_entity
-        if entity_version_pks is None:
-            entity_version_pks = [None] * len(publishable_entities_pks)
         entity_list = create_entity_list_with_rows(
-            entity_pks=publishable_entities_pks,
-            entity_version_pks=entity_version_pks,
+            entity_rows=entity_rows,
             learning_package_id=entity.learning_package_id,
         )
         container_version = _create_container_version(
@@ -785,8 +787,7 @@ class ChildrenEntitiesAction(Enum):
 def create_next_entity_list(
     learning_package_id: int,
     last_version: ContainerVersion,
-    publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None] | None,
+    entity_rows: list[ContainerEntityRow],
     entities_action: ChildrenEntitiesAction = ChildrenEntitiesAction.REPLACE,
 ) -> EntityList:
     """
@@ -795,55 +796,53 @@ def create_next_entity_list(
     Args:
         learning_package_id: Learning package ID
         last_version: Last version of container.
-        publishable_entities_pks: The IDs of the members current members of the container.
-        entity_version_pks: The IDs of the versions to pin to, if pinning is desired.
+        entity_rows: List of ContainerEntityRows specifying the publishable entity ID and version ID (if pinned).
         entities_action: APPEND, REMOVE or REPLACE given entities from/to the container
 
     Returns:
         The newly created entity list.
     """
-    if entity_version_pks is None:
-        entity_version_pks: list[int | None] = [None] * len(publishable_entities_pks)  # type: ignore[no-redef]
     if entities_action == ChildrenEntitiesAction.APPEND:
         # get previous entity list rows
         last_entities = last_version.entity_list.entitylistrow_set.only(
             "entity_id",
             "entity_version_id"
         ).order_by("order_num")
-        # append given publishable_entities_pks and entity_version_pks
-        publishable_entities_pks = [entity.entity_id for entity in last_entities] + publishable_entities_pks
-        entity_version_pks = [  # type: ignore[operator, assignment]
-            entity.entity_version_id
+        # append given entity_rows to the existing children
+        entity_rows = [
+            ContainerEntityRow(
+                entity_pk=entity.entity_id,
+                version_pk=entity.entity_version_id,
+            )
             for entity in last_entities
-        ] + entity_version_pks
+        ] + entity_rows
     elif entities_action == ChildrenEntitiesAction.REMOVE:
-        # get previous entity list rows
+        # get previous entity list, excluding the entities in entity_rows
         last_entities = last_version.entity_list.entitylistrow_set.only(
             "entity_id",
             "entity_version_id"
+        ).exclude(
+            entity_id__in=[entity.entity_pk for entity in entity_rows]
         ).order_by("order_num")
-        # Remove entities that are in publishable_entities_pks
-        new_entities = [
-            entity
-            for entity in last_entities
-            if entity.entity_id not in publishable_entities_pks
+        entity_rows = [
+            ContainerEntityRow(
+                entity_pk=entity.entity_id,
+                version_pk=entity.entity_version_id,
+            )
+            for entity in last_entities.all()
         ]
-        publishable_entities_pks = [entity.entity_id for entity in new_entities]
-        entity_version_pks = [entity.entity_version_id for entity in new_entities]
-    next_entity_list = create_entity_list_with_rows(
-        entity_pks=publishable_entities_pks,
-        entity_version_pks=entity_version_pks,  # type: ignore[arg-type]
+
+    return create_entity_list_with_rows(
+        entity_rows=entity_rows,
         learning_package_id=learning_package_id,
     )
-    return next_entity_list
 
 
 def create_next_container_version(
     container_pk: int,
     *,
     title: str | None,
-    publishable_entities_pks: list[int] | None,
-    entity_version_pks: list[int | None] | None,
+    entity_rows: list[ContainerEntityRow] | None,
     created: datetime,
     created_by: int | None,
     container_version_cls: type[ContainerVersionModel] = ContainerVersion,  # type: ignore[assignment]
@@ -863,8 +862,8 @@ 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. None to keep the current title.
-        publishable_entities_pks: The IDs of the members current members of the container. Or None for no change.
-        entity_version_pks: The IDs of the versions to pin to, if pinning is desired.
+        entity_rows: List of ContainerEntityRows specifying the publishable entity ID and version ID (if pinned).
+        Or None for no change.
         created: The date and time the container version was created.
         created_by: The ID of the user who created the container version.
         container_version_cls: The subclass of ContainerVersion to use, if applicable.
@@ -879,15 +878,15 @@ def create_next_container_version(
         last_version = container.versioning.latest
         assert last_version is not None
         next_version_num = last_version.version_num + 1
-        if publishable_entities_pks is None:
+
+        if entity_rows is None:
             # We're only changing metadata. Keep the same entity list.
             next_entity_list = last_version.entity_list
         else:
             next_entity_list = create_next_entity_list(
                 entity.learning_package_id,
                 last_version,
-                publishable_entities_pks,
-                entity_version_pks,
+                entity_rows,
                 entities_action
             )
 
@@ -969,6 +968,23 @@ def entity(self):
         return self.entity_version.entity
 
 
+@dataclass(frozen=True, kw_only=True, slots=True)
+class ContainerEntityRow:
+    """
+    [ 🛑 UNSTABLE ]
+    Used to specify the primary key of PublishableEntity and optional PublishableEntityVersion.
+
+    If version_pk is None (default), then the entity is considered "unpinned",
+    meaning that the latest version of the entity will be used.
+    """
+    entity_pk: int
+    version_pk: int | None = None
+
+    @property
+    def pinned(self):
+        return self.entity_pk and self.version_pk is not None
+
+
 def get_entities_in_container(
     container: Container,
     *,

openedx_learning/apps/authoring/units/api.py

@@ -62,8 +62,7 @@ def create_unit_version(
     version_num: int,
     *,
     title: str,
-    publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None],
+    entity_rows: list[publishing_api.ContainerEntityRow],
     created: datetime,
     created_by: int | None = None,
 ) -> UnitVersion:
@@ -75,20 +74,18 @@ def create_unit_version(
     `create_next_unit_version()` instead.
 
     Args:
-        unit_pk: The unit ID.
+        unit: The unit object.
         version_num: The version number.
         title: The title.
-        publishable_entities_pk: The publishable entities.
-        entity: The entity.
+        entity_rows: child entities/versions
         created: The creation date.
         created_by: The user who created the unit.
     """
     return publishing_api.create_container_version(
         unit.pk,
         version_num,
         title=title,
-        publishable_entities_pks=publishable_entities_pks,
-        entity_version_pks=entity_version_pks,
+        entity_rows=entity_rows,
         created=created,
         created_by=created_by,
         container_version_cls=UnitVersion,
@@ -97,30 +94,34 @@ def create_unit_version(
 
 def _pub_entities_for_components(
     components: list[Component | ComponentVersion] | None,
-) -> tuple[list[int], list[int | None]] | tuple[None, None]:
+) -> list[publishing_api.ContainerEntityRow] | None:
     """
     Helper method: given a list of Component | ComponentVersion, return the
-    lists of publishable_entities_pks and entity_version_pks needed for the
-    base container APIs.
+    list of ContainerEntityRows needed for the base container APIs.
 
     ComponentVersion is passed when we want to pin a specific version, otherwise
     Component is used for unpinned.
     """
     if components is None:
         # When these are None, that means don't change the entities in the list.
-        return None, None
+        return None
     for c in components:
         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)
+    return [
+        (
+            publishing_api.ContainerEntityRow(
+                entity_pk=c.publishable_entity_id,
+                version_pk=None,
+            ) if isinstance(c, Component)
+            else  # isinstance(c, ComponentVersion)
+            publishing_api.ContainerEntityRow(
+                entity_pk=c.component.publishable_entity_id,
+                version_pk=c.pk,
+            )
+        )
         for c in components
     ]
-    entity_version_pks = [
-        (cv.pk if isinstance(cv, ComponentVersion) else None)
-        for cv in components
-    ]
-    return publishable_entities_pks, entity_version_pks
 
 
 def create_next_unit_version(
@@ -143,12 +144,11 @@ def create_next_unit_version(
         created: The creation date.
         created_by: The user who created the unit.
     """
-    publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components)
+    entity_rows = _pub_entities_for_components(components)
     unit_version = publishing_api.create_next_container_version(
         unit.pk,
         title=title,
-        publishable_entities_pks=publishable_entities_pks,
-        entity_version_pks=entity_version_pks,
+        entity_rows=entity_rows,
         created=created,
         created_by=created_by,
         container_version_cls=UnitVersion,
@@ -177,7 +177,7 @@ def create_unit_and_version(
         created_by: The user who created the unit.
         can_stand_alone: Set to False when created as part of containers
     """
-    publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components)
+    entity_rows = _pub_entities_for_components(components)
     with atomic():
         unit = create_unit(
             learning_package_id,
@@ -190,8 +190,7 @@ def create_unit_and_version(
             unit,
             1,
             title=title,
-            publishable_entities_pks=publishable_entities_pks or [],
-            entity_version_pks=entity_version_pks or [],
+            entity_rows=entity_rows or [],
             created=created,
             created_by=created_by,
         )