feat: allow changing container title/metadata without changing its list

Author: bradenmacdonald

openedx_learning/apps/authoring/containers/api.py

@@ -110,44 +110,13 @@ def create_entity_list_with_rows(
     return entity_list
 
 
-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:
-        entity_list: The entity list to copy the rows to.
-        rows: The rows to copy to the new entity list.
-
-    Returns:
-        The newly created entity list.
-    """
-    entity_list = create_entity_list()
-    with atomic():
-        _ = EntityListRow.objects.bulk_create(
-            [
-                EntityListRow(
-                    entity_list=entity_list,
-                    entity_id=row.entity.id,
-                    order_num=row.order_num,
-                    entity_version_id=None,  # For simplicity, we are not copying the pinned versions
-                )
-                for row in rows
-            ]
-        )
-
-    return entity_list
-
-
 def create_container_version(
     container_pk: int,
     version_num: int,
     *,
     title: str,
     publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None],
+    entity_version_pks: list[int | None] | None,
     created: datetime,
     created_by: int | None,
 ) -> ContainerEntityVersion:
@@ -169,31 +138,45 @@ def create_container_version(
     with atomic():
         container = ContainerEntity.objects.select_related("publishable_entity").get(pk=container_pk)
         entity = container.publishable_entity
+
+        # 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.")
+
+        assert title is not None
+        assert publishable_entities_pks is not None
+        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,
+        )
         publishable_entity_version = publishing_api.create_publishable_entity_version(
             entity.pk,
             version_num=version_num,
             title=title,
             created=created,
             created_by=created_by,
         )
-        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,
             entity_list=entity_list,
         )
+
     return container_version
 
 
 def create_next_container_version(
     container_pk: int,
     *,
-    title: str,
-    publishable_entities_pks: list[int],
-    entity_version_pks: list[int | None],
+    title: str | None,
+    publishable_entities_pks: list[int] | None,
+    entity_version_pks: list[int | None] | None,
     created: datetime,
     created_by: int | None,
 ) -> ContainerEntityVersion:
@@ -210,37 +193,45 @@ 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_pks: The IDs of the members current members of the container.
+        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.
         created: The date and time the container version was created.
         created_by: The ID of the user who created the 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.select_related("publishable_entity").get(pk=container_pk)
         entity = container.publishable_entity
         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:
+            # We're only changing metadata. Keep the same entity list.
+            next_entity_list = last_version.entity_list
+        else:
+            # 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.")
+            if entity_version_pks is None:
+                entity_version_pks = [None] * len(publishable_entities_pks)
+            next_entity_list = create_entity_list_with_rows(
+                entity_pks=publishable_entities_pks,
+                entity_version_pks=entity_version_pks,
+            )
         publishable_entity_version = publishing_api.create_publishable_entity_version(
             entity.pk,
             version_num=next_version_num,
-            title=title,
+            title=title if title is not None else last_version.title,
             created=created,
             created_by=created_by,
         )
-        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,

openedx_learning/apps/authoring/units/api.py

@@ -95,8 +95,9 @@ def create_unit_version(
 
 def create_next_unit_version(
     unit: Unit,
-    title: str,
-    components: list[Component | ComponentVersion],
+    *,
+    title: str | None = None,
+    components: list[Component | ComponentVersion] | None = None,
     created: datetime,
     created_by: int | None = None,
 ) -> UnitVersion:
@@ -105,25 +106,29 @@ def create_next_unit_version(
 
     Args:
         unit_pk: The unit ID.
-        title: The title.
-        components: The components, as a list of Components (unpinned) and/or ComponentVersions (pinned)
+        title: The title. Leave as None to keep the current title.
+        components: The components, as a list of Components (unpinned) and/or ComponentVersions (pinned). Passing None
+           will leave the existing components unchanged.
         created: The creation date.
         created_by: The user who created the unit.
     """
-    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)
-        for c in components
-    ]
-    entity_version_pks = [
-        (cv.pk if isinstance(cv, ComponentVersion) else None)
-        for cv in components
-    ]
+    if components is not 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)
+            for c in components
+        ]
+        entity_version_pks = [
+            (cv.pk if isinstance(cv, ComponentVersion) else None)
+            for cv in components
+        ]
+    else:
+        # When these are None, that means don't change the entities in the list.
+        publishable_entities_pks = None
+        entity_version_pks = None
     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.
         container_entity_version = container_api.create_next_container_version(
             unit.container_entity.pk,
             title=title,

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

@@ -554,6 +554,26 @@ def test_query_count_of_contains_unpublished_changes(self):
         with self.assertNumQueries(2):
             assert authoring_api.contains_unpublished_changes(unit) is True
 
+    def test_metadata_change_doesnt_create_entity_list(self):
+        """
+        Test that changing a container's metadata like title will create a new
+        version, but can re-use the same EntityList. API consumers generally
+        shouldn't depend on this behavior; it's an optimization.
+        """
+        unit = self.create_unit_with_components([self.component_1, self.component_2_v1])
+
+        orig_version_num = unit.container_entity.versioning.draft.version_num
+        orig_entity_list_id = unit.container_entity.versioning.draft.entity_list.pk
+
+        authoring_api.create_next_unit_version(unit, title="New Title", created=self.now)
+
+        unit.refresh_from_db()
+        new_version_num = unit.container_entity.versioning.draft.version_num
+        new_entity_list_id = unit.container_entity.versioning.draft.entity_list.pk
+
+        assert new_version_num > orig_version_num
+        assert new_entity_list_id == orig_entity_list_id
+
     @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):