From 1f10aec16926b1131884235b0f9ee1a62e02ed88 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 8 Nov 2024 13:20:03 +0100 Subject: [PATCH] refactor: pin versions in frozen list each time a new version is created --- .../apps/authoring/containers/api.py | 148 ++++++++++++------ openedx_learning/apps/authoring/units/api.py | 16 +- 2 files changed, 109 insertions(+), 55 deletions(-) diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py index 4a66e748..2920d037 100644 --- a/openedx_learning/apps/authoring/containers/api.py +++ b/openedx_learning/apps/authoring/containers/api.py @@ -70,34 +70,42 @@ def create_entity_list() -> EntityList: return EntityList.objects.create() -def create_entity_list_row( +def create_entity_list_rows( entity_list: EntityList, - entity_pk: int, - order_num: int, - draft_version_pk: int | None, - published_version_pk: int | None, + entity_pks: list[int], + draft_version_pks: list[int | None], + published_version_pks: list[int | None], ) -> EntityListRow: """ - Create a new entity list row. This is a row in an entity list that references - publishable entities. + Create new entity list rows for an entity list. Args: - entity_list: The entity list that the entity list row belongs to. - entity: The ID of the publishable entity that the entity list row references. - order_num: The order_num of the entity list row in the entity list. - draft_version_pk: The ID of the draft version of the entity (PublishableEntityVersion) that the entity list row references. - published_version_pk: The ID of the published version of the entity (PublishableEntityVersion) that the entity list row references + 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. Returns: - The newly created entity list row. + The newly created entity list rows. """ - return EntityListRow.objects.create( - entity_list=entity_list, - entity_id=entity_pk, - order_num=order_num, - draft_version_id=draft_version_pk, - published_version_id=published_version_pk, - ) + order_nums = range(len(entity_pks)) + with atomic(): + entity_list_rows = EntityListRow.objects.bulk_create( + [ + EntityListRow( + entity_list=entity_list, + entity_id=entity_pk, + order_num=order_num, + draft_version_id=draft_version_pk, + published_version_id=published_version_pk, + ) + for entity_pk, order_num, draft_version_pk, published_version_pk in zip( + entity_pks, order_nums, draft_version_pks, published_version_pks + ) + ] + ) + + return entity_list_rows def create_entity_list_with_rows( @@ -109,26 +117,55 @@ def create_entity_list_with_rows( Create a new entity list with rows. Args: - entity_pks: The IDs of the publishable entities that the entity list rows reference. + entity_pks: The IDs of the publishable entities that the entity list rows + reference. order_nums: The order_nums of the entity list rows in the entity list. - 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. + 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. Returns: The newly created entity list. """ entity_list = create_entity_list() - order_nums = range(len(entity_pks)) - for entity_pk, order_num, draft_version_pk, published_version_pk in zip( - entity_pks, order_nums, draft_version_pks, published_version_pks - ): - create_entity_list_row( - entity_list=entity_list, - entity_pk=entity_pk, - order_num=order_num, - draft_version_pk=draft_version_pk, - published_version_pk=published_version_pk, + create_entity_list_rows( + entity_list=entity_list, + entity_pks=entity_pks, + draft_version_pks=draft_version_pks, + published_version_pks=published_version_pks, + ) + return entity_list + + +def copy_rows_to_new_entity_list( + rows: QuerySet[EntityListRow], +) -> EntityList: + """ + 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(): + entity_list_rows = EntityListRow.objects.bulk_create( + [ + EntityListRow( + entity_list=entity_list, + entity_id=row.entity.id, + order_num=row.order_num, + draft_version_id=row.entity.draft.version.pk, + published_version_id=row.entity.published.version.pk, + ) + for row in rows + ] ) + return entity_list @@ -137,6 +174,8 @@ 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: PublishableEntity, created: datetime, created_by: int | None, @@ -164,20 +203,17 @@ def create_container_version( created=created, created_by=created_by, ) - # This implementation assumes: - # 1. We are creating the first version of the container, so the defined list is the same as the initial list. - # 2. The frozen list is empty because this is the first version. - # 3. Published and draft versions are always the latest for all members. entity_list = create_entity_list_with_rows( entity_pks=publishable_entities_pk, - draft_version_pks=[None] * len(publishable_entities_pk), - published_version_pks=[None] * len(publishable_entities_pk), + draft_version_pks=draft_version_pks, + published_version_pks=published_version_pks, ) container_version = ContainerEntityVersion.objects.create( publishable_entity_version=publishable_entity_version, container_id=container_pk, defined_list=entity_list, initial_list=entity_list, + # Would frozen_list be always None the 1st time a container is created? frozen_list=None, ) return container_version @@ -187,6 +223,8 @@ 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: PublishableEntity, created: datetime, created_by: int | None, @@ -216,23 +254,26 @@ def create_next_container_version( created=created, created_by=created_by, ) - # This implementation assumes: - # 1. The changes provoking a new version are the addition, removal of members or reordering. + # 1. The changes provoking a new version are always the addition, removal of members or reordering. How can we detect changes only in the metadata? # 2. Published and draft versions are always the latest for all members. # 3. When creating a new version, a new user-defined entity list is created to preserve the latest state as the previous user-defined list. - # TODO: instead consider copying the previous user-defined list as the frozen list, and add/remove to the previous user-defined list. - # If it's a reordering, the previous user-defined list is copied as the frozen and a new user-defined list is created with the new order. + # 4. When creating a new version, a new frozen entity list is created copying the state of the user-defined list of the previous version. + # 5. While copying the versions into the new frozen list, versions are pinned in new rows for published/draft versions. new_user_defined_list = create_entity_list_with_rows( entity_pks=publishable_entities_pk, - draft_version_pks=[None] * len(publishable_entities_pk), - published_version_pks=[None] * len(publishable_entities_pk), + draft_version_pks=draft_version_pks, + published_version_pks=published_version_pks, ) + new_frozen_list = copy_rows_to_new_entity_list( + rows=get_defined_list_rows_for_container_version(last_version) + ) + container_version = ContainerEntityVersion.objects.create( publishable_entity_version=publishable_entity_version, container_id=container_pk, defined_list=new_user_defined_list, initial_list=last_version.initial_list, - frozen_list=last_version.defined_list, + frozen_list=new_frozen_list, ) return container_version @@ -244,6 +285,8 @@ 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], ) -> ContainerEntityVersion: """ Create a new container and its first version. @@ -267,6 +310,8 @@ 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=container.publishable_entity, created=created, created_by=created_by, @@ -283,13 +328,12 @@ def get_container(pk: int) -> ContainerEntity: Returns: The container with the given primary key. - - TODO: should this use with_publishing_relations as in components? """ + # TODO: should this use with_publishing_relations as in components? return ContainerEntity.objects.get(pk=pk) -def get_defined_list_for_container_version( +def get_defined_list_rows_for_container_version( container_version: ContainerEntityVersion, ) -> QuerySet[EntityListRow]: """ @@ -304,7 +348,7 @@ def get_defined_list_for_container_version( return container_version.defined_list.entitylistrow_set.all() -def get_initial_list_for_container_version( +def get_initial_list_rows_for_container_version( container_version: ContainerEntityVersion, ) -> QuerySet[EntityListRow]: """ @@ -319,7 +363,7 @@ def get_initial_list_for_container_version( return container_version.initial_list.entitylistrow_set.all() -def get_frozen_list_for_container_version( +def get_frozen_list_rows_for_container_version( container_version: ContainerEntityVersion, ) -> QuerySet[EntityListRow]: """ @@ -331,4 +375,6 @@ def get_frozen_list_for_container_version( 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() diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 9e06a8ad..9982354c 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -54,7 +54,9 @@ def create_unit_version( unit: Unit, version_num: int, title: str, - publishable_entities_pk: list[int], + publishable_entities_pks: list[int], + draft_version_pks: list[int | None], + published_version_pks: list[int | None], created: datetime, created_by: int | None, ) -> Unit: @@ -74,7 +76,9 @@ def create_unit_version( unit.container_entity.pk, version_num, title, - publishable_entities_pk, + publishable_entities_pks, + draft_version_pks, + published_version_pks, unit.container_entity.publishable_entity, created, created_by, @@ -90,7 +94,9 @@ def create_unit_version( def create_next_unit_version( unit: Unit, title: str, - publishable_entities_pk: list[int], + publishable_entities_pks: list[int], + draft_version_pks: list[int | None], + published_version_pks: list[int | None], created: datetime, created_by: int | None, ) -> Unit: @@ -110,7 +116,9 @@ def create_next_unit_version( container_entity_version = container_api.create_next_container_version( unit.container_entity.pk, title, - publishable_entities_pk, + publishable_entities_pks, + draft_version_pks, + published_version_pks, unit.container_entity.publishable_entity, created, created_by,