refactor: update child entities to new API, fix some name & style issues

Author: Andrii

openedx_learning/apps/authoring/publishing/api.py

@@ -1059,9 +1059,8 @@ def contains_unpublished_changes(container_id: int) -> bool:
 
     # We only care about children that are un-pinned, since published changes to pinned children don't matter
     entity_list = getattr(container.versioning.draft, "entity_list", None)
-    # ?FOR REVIEW: Is this correct?
     if entity_list is None:
-        # This container has not been published yet, or has been deleted.
+        # This container has been soft-deleted, so it has no children.
         return False
 
     # This is a naive and inefficient implementation but should be correct.

openedx_learning/apps/authoring/subsections/api.py

@@ -62,8 +62,7 @@ def create_subsection_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,
 ) -> SubsectionVersion:
@@ -78,17 +77,15 @@ def create_subsection_version(
         subsection_pk: The subsection ID.
         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 subsection.
     """
     return publishing_api.create_container_version(
         subsection.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=SubsectionVersion,
@@ -97,30 +94,33 @@ def create_subsection_version(
 
 def _pub_entities_for_units(
     units: list[Unit | UnitVersion] | None,
-) -> tuple[list[int], list[int | None]] | tuple[None, None]:
+) -> list[publishing_api.ContainerEntityRow] | None:
     """
     Helper method: given a list of Unit | UnitVersion, 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.
 
     UnitVersion is passed when we want to pin a specific version, otherwise
     Unit is used for unpinned.
     """
     if units is None:
         # When these are None, that means don't change the entities in the list.
-        return None, None
+        return None
     for u in units:
         if not isinstance(u, (Unit, UnitVersion)):
             raise TypeError("Subsection units must be either Unit or UnitVersion.")
-    publishable_entities_pks = [
-        (u.publishable_entity_id if isinstance(u, Unit) else u.unit.publishable_entity_id)
+    return [
+        (
+            publishing_api.ContainerEntityRow(
+                entity_pk=u.container.publishable_entity_id,
+                version_pk=None,
+            ) if isinstance(u, Unit)
+            else publishing_api.ContainerEntityRow(
+                entity_pk=u.unit.container.publishable_entity_id,
+                version_pk=u.container_version.publishable_entity_version_id,
+            )
+        )
         for u in units
     ]
-    entity_version_pks = [
-        (uv.pk if isinstance(uv, UnitVersion) else None)
-        for uv in units
-    ]
-    return publishable_entities_pks, entity_version_pks
 
 
 def create_next_subsection_version(
@@ -143,12 +143,11 @@ def create_next_subsection_version(
         created: The creation date.
         created_by: The user who created the subsection.
     """
-    publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units)
+    entity_rows = _pub_entities_for_units(units)
     subsection_version = publishing_api.create_next_container_version(
         subsection.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=SubsectionVersion,
@@ -177,7 +176,7 @@ def create_subsection_and_version(
         created_by: The user who created the subsection.
         can_stand_alone: Set to False when created as part of containers
     """
-    publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units)
+    entity_rows = _pub_entities_for_units(units)
     with atomic():
         subsection = create_subsection(
             learning_package_id,
@@ -190,8 +189,7 @@ def create_subsection_and_version(
             subsection,
             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,
         )
@@ -286,7 +284,9 @@ def get_units_in_published_subsection_as_of(
           ancestors of every modified PublishableEntity in the publish.
     """
     assert isinstance(subsection, Subsection)
-    subsection_pub_entity_version = publishing_api.get_published_version_as_of(subsection.publishable_entity_id, publish_log_id)
+    subsection_pub_entity_version = publishing_api.get_published_version_as_of(
+        subsection.publishable_entity_id, publish_log_id
+    )
     if subsection_pub_entity_version is None:
         return None  # This subsection was not published as of the given PublishLog ID.
     container_version = subsection_pub_entity_version.containerversion
@@ -303,5 +303,7 @@ def get_units_in_published_subsection_as_of(
             # This is not optimized. It could be done in one query per subsection rather than one query per unit.
             pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id)
             if pub_entity_version:
-                entity_list.append(SubsectionListEntry(unit_version=pub_entity_version.containerversion.unitversion, pinned=False))
+                entity_list.append(
+                    SubsectionListEntry(unit_version=pub_entity_version.containerversion.unitversion, pinned=False)
+                )
     return entity_list

openedx_learning/apps/authoring/subsections/models.py

@@ -1,5 +1,5 @@
 """
-Models that implement units
+Models that implement subsections
 """
 from django.db import models
 

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

@@ -1,6 +1,8 @@
 """
-Basic tests for the units API.
+Basic tests for the subsections API.
 """
+from unittest.mock import patch
+
 import ddt  # type: ignore[import]
 import pytest
 from django.core.exceptions import ValidationError
@@ -180,7 +182,7 @@ def test_create_subsection_queries(self):
         # The exact numbers here aren't too important - this is just to alert us if anything significant changes.
         with self.assertNumQueries(22):
             _empty_subsection = self.create_subsection_with_units([])
-        with self.assertNumQueries(26):
+        with self.assertNumQueries(27):
             # And try with a non-empty subsection:
             self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2")
 
@@ -243,7 +245,28 @@ def test_adding_external_units(self):
                 created_by=None,
             )
 
-    # ?FOR REVIEW
+    @patch('openedx_learning.apps.authoring.subsections.api._pub_entities_for_units')
+    def test_adding_mismatched_versions(self, mock_entities_for_units):
+        """
+        Test that versioned units must match their entities.
+        """
+        mock_entities_for_units.return_value = [
+            authoring_api.ContainerEntityRow(
+                entity_pk=self.unit_1.pk,
+                version_pk=self.unit_2_v1.pk,
+            ),
+        ]
+        # Try adding a a unit from LP 1 (self.learning_package) to a subsection from LP 2
+        with pytest.raises(ValidationError, match="Container entity versions must belong to the specified entity"):
+            authoring_api.create_subsection_and_version(
+                learning_package_id=self.unit_1.container.publishable_entity.learning_package.pk,
+                key="subsection:key",
+                title="Subsection",
+                units=[self.unit_1],
+                created=self.now,
+                created_by=None,
+            )
+
     @ddt.data(True, False)
     @pytest.mark.skip(reason="FIXME: This test is failing because the publishable_entity is not deleted from the database with the unit.")
     # FIXME: Also, exception is Container.DoesNotExist, not Unit.DoesNotExist
@@ -360,7 +383,9 @@ def test_auto_publish_children(self):
         # Publish ONLY the subsection. This should however also auto-publish units 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=subsection.publishable_entity),
+            draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter(
+                entity=subsection.publishable_entity
+            ),
         )
         # Now all changes to the subsection and to unit 1 are published:
         subsection.refresh_from_db()
@@ -976,7 +1001,9 @@ def test_subsections_containing(self):
         with self.assertNumQueries(1):
             result2 = [
                 c.subsection for c in
-                authoring_api.get_containers_with_entity(self.unit_1.pk, ignore_pinned=True).select_related("subsection")
+                authoring_api.get_containers_with_entity(
+                    self.unit_1.pk, ignore_pinned=True
+                ).select_related("subsection")
             ]
         assert result2 == [subsection4_unpinned]