From 705b319295780631c8daffc13d1734422489b408 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 9 Apr 2025 16:00:58 +0930 Subject: [PATCH 1/4] refactor!: use ContainerEntityRow in container API instead of passing two separate publishable_entities_pks + entity_version_pks lists as arguments. ContainerEntityRow encapsulates the entity.pk + (optional) entity version.pk in a single structure. --- .../apps/authoring/publishing/api.py | 102 +++++++++--------- openedx_learning/apps/authoring/units/api.py | 47 ++++---- 2 files changed, 74 insertions(+), 75 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index e9641dc1..82be3f24 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/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,13 +659,12 @@ 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)) with atomic(savepoint=False): entity_list = create_entity_list() @@ -676,13 +672,11 @@ def create_entity_list_with_rows( [ 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 +719,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 +732,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 +741,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 +774,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 +783,54 @@ 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 last_entities = last_version.entity_list.entitylistrow_set.only( "entity_id", "entity_version_id" ).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 + # Remove existing children found in entity_rows + remove_entity_pks = [entity.entity_pk for entity in entity_rows] + entity_rows = [ + ContainerEntityRow( + entity_pk=entity.entity_id, + version_pk=entity.entity_version_id, + ) + for entity in last_entities.all() + if entity.entity_id not in remove_entity_pks ] - 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 +850,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 +866,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 +956,19 @@ 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 + + def get_entities_in_container( container: Container, *, diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 6d19306d..58dd6e37 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/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,11 +74,10 @@ 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. """ @@ -87,8 +85,7 @@ def create_unit_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, ) From 28f8a159e600a5b6e92c2f5fd86c04f3ff87b0f4 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 9 Apr 2025 16:04:20 +0930 Subject: [PATCH 2/4] chore: bumps package version --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 5f85ec83..f621aeeb 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.19.2" +__version__ = "0.19.3" From 8422317354ffc530c8b6374a72a0ff6595469700 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 10 Apr 2025 19:37:57 +0930 Subject: [PATCH 3/4] feat: adds check to ensure pinned entity versions match their entities when adding/updating container children --- .../apps/authoring/publishing/api.py | 17 ++++++++++++ .../apps/authoring/units/test_api.py | 26 ++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 82be3f24..ef091c6e 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -665,6 +665,19 @@ def create_entity_list_with_rows( ).exists(): raise ValidationError("Container entities must be from the same learning package.") + # 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() @@ -968,6 +981,10 @@ class ContainerEntityRow: 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, diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index a07024db..c482f937 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. """ +from unittest.mock import patch + import ddt # type: ignore[import] import pytest from django.core.exceptions import ValidationError @@ -171,7 +173,7 @@ def test_create_unit_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_unit = self.create_unit_with_components([]) - with self.assertNumQueries(25): + with self.assertNumQueries(26): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2") @@ -234,6 +236,28 @@ def test_adding_external_components(self): created_by=None, ) + @patch('openedx_learning.apps.authoring.units.api._pub_entities_for_components') + def test_adding_mismatched_versions(self, mock_entities_for_components): + """ + Test that versioned components must match their entities. + """ + mock_entities_for_components.return_value = [ + authoring_api.ContainerEntityRow( + entity_pk=self.component_1.pk, + version_pk=self.component_2_v1.pk, + ), + ] + # Try adding a a component from LP 1 (self.learning_package) to a unit from LP 2 + with pytest.raises(ValidationError, match="Container entity versions must belong to the specified entity"): + authoring_api.create_unit_and_version( + learning_package_id=self.component_1.learning_package.id, + key="unit:key", + title="Unit", + components=[self.component_1], + created=self.now, + created_by=None, + ) + @ddt.data(True, False) def test_cannot_add_invalid_ids(self, pin_version): """ From 84dc6f7642aa285ae378478685a22ebd2ba561b8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 10 Apr 2025 19:41:12 +0930 Subject: [PATCH 4/4] refactor: filter excluded entities in query instead of in loop --- openedx_learning/apps/authoring/publishing/api.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index ef091c6e..b2f365e3 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -817,20 +817,19 @@ def create_next_entity_list( for entity in last_entities ] + 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 existing children found in entity_rows - remove_entity_pks = [entity.entity_pk for entity in entity_rows] entity_rows = [ ContainerEntityRow( entity_pk=entity.entity_id, version_pk=entity.entity_version_id, ) for entity in last_entities.all() - if entity.entity_id not in remove_entity_pks ] return create_entity_list_with_rows(