From e09a20e4bc05e6ec017a6d76353aeae0a3bd9c29 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 20 Feb 2025 16:46:15 -0800 Subject: [PATCH] refactor: consolidate defined_list/initial_list/frozen_list into entity_list --- .../apps/authoring/containers/api.py | 161 ++---------------- .../containers/migrations/0001_initial.py | 4 +- .../apps/authoring/containers/models.py | 46 +---- openedx_learning/apps/authoring/units/api.py | 2 +- .../apps/authoring/units/test_api.py | 64 +------ 5 files changed, 22 insertions(+), 255 deletions(-) diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py index 7df5fe0eb..0565dd046 100644 --- a/openedx_learning/apps/authoring/containers/api.py +++ b/openedx_learning/apps/authoring/containers/api.py @@ -73,9 +73,7 @@ def create_entity_list() -> EntityList: return EntityList.objects.create() -def create_next_defined_list( - previous_entity_list: EntityList | None, # pylint: disable=unused-argument - new_entity_list: EntityList, +def create_entity_list_with_rows( entity_pks: list[int], entity_version_pks: list[int | None], ) -> EntityList: @@ -84,52 +82,6 @@ def create_next_defined_list( Create new entity list rows for an entity list. Args: - previous_entity_list: The previous entity list that the new entity list is based on. - new_entity_list: The entity list to create the rows for. - entity_pks: The IDs of the publishable entities that the entity list rows reference. - entity_version_pks: The IDs of the draft versions of the entities - (PublishableEntityVersion) that the entity list rows reference, or - Nones for "unpinned" (default). - - Returns: - The newly created entity list rows. - """ - order_nums = range(len(entity_pks)) - with atomic(): - # Case 1: create first container version (no previous rows created for container) - # 1. Create new rows for the entity list - # Case 2: create next container version (previous rows created for container) - # 1. Get all the rows in the previous version entity list - # 2. Only associate existent rows to the new entity list iff: the order is the same, the PublishableEntity is in - # entity_pks and versions are not pinned - # 3. If the order is different for a row with the PublishableEntity, create new row with the same - # PublishableEntity for the new order and associate the new row to the new entity list - new_rows = [] - for order_num, entity_pk, entity_version_pk in zip( - order_nums, entity_pks, entity_version_pks - ): - new_rows.append( - EntityListRow( - entity_list=new_entity_list, - entity_id=entity_pk, - order_num=order_num, - entity_version_id=entity_version_pk, - ) - ) - EntityListRow.objects.bulk_create(new_rows) - return new_entity_list - - -def create_defined_list_with_rows( - entity_pks: list[int], - entity_version_pks: list[int | None], -) -> EntityList: - """ - [ ๐Ÿ›‘ UNSTABLE ] - Create new entity list rows for an entity list. - - Args: - entity_list: The entity list to create the rows for. 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 @@ -188,46 +140,6 @@ def get_entity_list_with_pinned_versions( return entity_list -def check_unpinned_versions_in_defined_list( - defined_list: EntityList, -) -> bool: - """ - [ ๐Ÿ›‘ UNSTABLE ] - Check if there are any unpinned versions in the defined list. - - Args: - defined_list: The defined list to check for unpinned versions. - - Returns: - True if there are unpinned versions in the defined list, False otherwise. - """ - # Is there a way to short-circuit this? - return any( - row.entity_version is None - for row in defined_list.entitylistrow_set.all() - ) - - -def check_new_changes_in_defined_list( - entity_list: EntityList, # pylint: disable=unused-argument - publishable_entities_pks: list[int], # pylint: disable=unused-argument -) -> bool: - """ - [ ๐Ÿ›‘ UNSTABLE ] - Check if there are any new changes in the defined list. - - Args: - entity_list: The entity list to check for new changes. - publishable_entities: The publishable entities to check for new changes. - - Returns: - True if there are new changes in the defined list, False otherwise. - """ - # Is there a way to short-circuit this? Using queryset operations - # For simplicity, return True - return True - - def create_container_version( container_pk: int, version_num: int, @@ -263,19 +175,14 @@ def create_container_version( created=created, created_by=created_by, ) - defined_list = create_defined_list_with_rows( + 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, - defined_list=defined_list, - initial_list=defined_list, - # TODO: Check for unpinned versions in defined_list to know whether to point this to the defined_list - # point to None. - # If this is the first version ever created for this ContainerEntity, then start as None. - frozen_list=None, + entity_list=entity_list, ) return container_version @@ -330,50 +237,14 @@ def create_next_container_version( created=created, created_by=created_by, ) - # 1. Check if there are any changes in the container's members - # 2. Pin versions in previous frozen list for last container version - # 3. Create new defined list for author changes - # 4. Pin versions in defined list to create initial list - # 5. Check for unpinned references in defined_list to determine if frozen_list should be None - # 6. Point frozen_list to None or defined_list - if check_new_changes_in_defined_list( - entity_list=last_version.defined_list, - publishable_entities_pks=publishable_entities_pks, - ): - # Only change if there are unpin versions in defined list, meaning last frozen list is None - # When does this has to happen? Before? - if not last_version.frozen_list: - last_version.frozen_list = get_entity_list_with_pinned_versions( - rows=last_version.defined_list.entitylistrow_set.all() - ) - last_version.save() - next_defined_list = create_next_defined_list( - previous_entity_list=last_version.defined_list, - new_entity_list=create_entity_list(), - entity_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - ) - next_initial_list = get_entity_list_with_pinned_versions( - rows=next_defined_list.entitylistrow_set.all() - ) - if check_unpinned_versions_in_defined_list(next_defined_list): - next_frozen_list = None - else: - next_frozen_list = next_initial_list - else: - # Do I need to create new EntityList and copy rows? - # I do think so because frozen can change when creating a new version - # Does it need to change though? - # What would happen if I only change the title? - next_defined_list = last_version.defined_list - next_initial_list = last_version.initial_list - next_frozen_list = last_version.frozen_list + 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, - defined_list=next_defined_list, - initial_list=next_initial_list, - frozen_list=next_frozen_list, + entity_list=next_entity_list, ) return next_container_version @@ -461,8 +332,7 @@ def get_entities_in_draft_container( container = container.container_entity assert isinstance(container, ContainerEntity) entity_list = [] - defined_list = container.versioning.draft.defined_list - for row in defined_list.entitylistrow_set.order_by("order_num"): + for row in container.versioning.draft.entity_list.entitylistrow_set.order_by("order_num"): entity_version = row.entity_version or row.entity.draft.version if entity_version is not None: # As long as this hasn't been soft-deleted: entity_list.append(ContainerEntityListEntry( @@ -490,13 +360,8 @@ def get_entities_in_published_container( if cev is None: return None # There is no published version of this container. Should this be an exception? assert isinstance(cev, ContainerEntityVersion) - # TODO: do we ever need frozen_list? e.g. when accessing a historical version? - # Doesn't make a lot of sense when the versions of the container don't capture many of the changes to the contents, - # e.g. container version 1 had component version 1 through 50, and container version 2 had component versions 51 - # through 100, what is the point of tracking whether container version 1 "should" show v1 or v50 when they're wildly - # different? entity_list = [] - for row in cev.defined_list.entitylistrow_set.order_by("order_num"): + for row in cev.entity_list.entitylistrow_set.order_by("order_num"): entity_version = row.entity_version or row.entity.published.version if entity_version is not None: # As long as this hasn't been soft-deleted: entity_list.append(ContainerEntityListEntry( @@ -524,7 +389,7 @@ def contains_unpublished_changes( "publishable_entity", "publishable_entity__draft", "publishable_entity__draft__version", - "publishable_entity__draft__version__containerentityversion__defined_list", + "publishable_entity__draft__version__containerentityversion__entity_list", ).get(pk=container.container_entity_id) else: pass # TODO: select_related if we're given a raw ContainerEntity rather than a ContainerEntityMixin like Unit? @@ -534,12 +399,12 @@ def contains_unpublished_changes( return True # We only care about children that are un-pinned, since published changes to pinned children don't matter - defined_list = container.versioning.draft.defined_list + entity_list = container.versioning.draft.entity_list # TODO: This is a naive inefficient implementation but hopefully correct. # Once we know it's correct and have a good test suite, then we can optimize. # We will likely change to a tracking-based approach rather than a "scan for changes" based approach. - for row in defined_list.entitylistrow_set.filter(entity_version=None).select_related( + for row in entity_list.entitylistrow_set.filter(entity_version=None).select_related( "entity__containerentity", "entity__draft__version", "entity__published__version", diff --git a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py index be274d595..ab1068e2a 100644 --- a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/containers/migrations/0001_initial.py @@ -43,9 +43,7 @@ class Migration(migrations.Migration): fields=[ ('publishable_entity_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentityversion')), ('container', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_containers.containerentity')), - ('defined_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='defined_list', to='oel_containers.entitylist')), - ('frozen_list', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='frozen_list', to='oel_containers.entitylist')), - ('initial_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='initial_list', to='oel_containers.entitylist')), + ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='entity_list', to='oel_containers.entitylist')), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py index 6de71a213..9d4ec9905 100644 --- a/openedx_learning/apps/authoring/containers/models.py +++ b/openedx_learning/apps/authoring/containers/models.py @@ -21,7 +21,7 @@ class EntityList(models.Model): sometimes we'll want the same kind of data structure for things that we dynamically generate for individual students (e.g. Variants). EntityLists are anonymous in a senseโ€“they're pointed to by ContainerEntityVersions and - other models, rather than being looked up by their own identifers. + other models, rather than being looked up by their own identifiers. """ @@ -92,7 +92,7 @@ class ContainerEntityVersion(PublishableEntityVersionMixin): The last looks a bit odd, but it's because *how we've defined the Unit* has changed if we decide to explicitly pin a set of versions for the children, and then later change our minds and move to a different set. It also just - makes things easier to reason about if we say that defined_list never + makes things easier to reason about if we say that entity_list never changes for a given ContainerEntityVersion. """ @@ -102,46 +102,10 @@ class ContainerEntityVersion(PublishableEntityVersionMixin): related_name="versions", ) - # This is the EntityList that the author defines. This should never change, - # even if the things it references get soft-deleted (because we'll need to - # maintain it for reverts). - defined_list = models.ForeignKey( + # The list of entities (frozen and/or unfrozen) in this container + entity_list = models.ForeignKey( EntityList, on_delete=models.RESTRICT, null=False, - related_name="defined_list", - ) - - # inital_list is an EntityList where all the versions are pinned, to show - # what the exact versions of the children were at the time that the - # Container was created. We could technically derive this, but it would be - # awkward to query. - # - # If the Container was defined so that all references were pinned, then this - # can point to the exact same EntityList as defined_list. - initial_list = models.ForeignKey( - EntityList, - on_delete=models.RESTRICT, - null=False, - related_name="initial_list", - ) - - # This is the EntityList that's created when the next ContainerEntityVersion - # is created. All references in this list should be pinned, and it serves as - # "the last state the children were in for this version of the Container". - # If defined_list has only pinned references, this should point to the same - # EntityList as defined_list and initial_list. - # - # This value is mutable if and only if there are unpinned references in - # defined_list. In that case, frozen_list should start as None, and be - # updated to pin references when another version of this Container becomes - # the Draft version. But if this version ever becomes the Draft *again* - # (e.g. the user hits "discard changes" or some kind of revert happens), - # then we need to clear this back to None. - frozen_list = models.ForeignKey( - EntityList, - on_delete=models.RESTRICT, - null=True, - default=None, - related_name="frozen_list", + related_name="entity_list", ) diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 8b3f04481..e6db83168 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -283,7 +283,7 @@ def get_components_in_published_unit_as_of( unit_version = unit_pub_entity_version.unitversion # type: ignore[attr-defined] entity_list = [] - rows = unit_version.container_entity_version.defined_list.entitylistrow_set.order_by("order_num") + rows = unit_version.container_entity_version.entity_list.entitylistrow_set.order_by("order_num") for row in rows: if row.entity_version is not None: component_version = row.entity_version.componentversion diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index d4f8af58b..345f8b368 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -705,43 +705,7 @@ def test_next_version_with_different_different_title(self): 2. The unit version number is 2. 3. The unit version is in the unit's versions. 4. The unit version's title is different from the previous version. - 5. The user defined is the same as the previous version. - 6. The frozen list is empty. - """ - - def test_check_author_defined_list_matches_components(self): - """Test checking the author defined list matches the components. - - Expected results: - 1. The author defined list matches the components used to create the unit version. - """ - - def test_check_initial_list_matches_components(self): - """Test checking the initial list matches the components. - - Expected results: - 1. The initial list matches the components (pinned) used to create the unit version. - """ - - def test_check_frozen_list_is_none_floating_versions(self): - """Test checking the frozen list is None when floating versions are used in the author defined list. - - Expected results: - 1. The frozen list is None. - """ - - def test_check_frozen_list_when_next_version_is_created(self): - """Test checking the frozen list when a new version is created. - - Expected results: - 1. The frozen list has pinned versions of the user defined list from the previous version. - """ - - def test_check_lists_equal_when_pinned_versions(self): - """Test checking the lists are equal when pinned versions are used. - - Expected results: - 1. The author defined list == initial list == frozen list. + 5. The entity list is the same as the previous version. """ def test_publish_unit_version(self): @@ -768,29 +732,5 @@ def test_next_version_with_different_order(self): 1. A new unit version is created. 2. The unit version number is 2. 3. The unit version is in the unit's versions. - 4. The user defined list is different from the previous version. - 5. The initial list contains the pinned versions of the defined list. - 6. The frozen list is empty. - """ - - def test_soft_delete_component_from_units(self): - """Soft-delete a component from a unit. - - Expected result: - After soft-deleting the component (draft), a new unit version (draft) is created for the unit. - """ - - def test_soft_delete_component_from_units_and_publish(self): - """Soft-delete a component from a unit and publish the unit. - - Expected result: - After soft-deleting the component (draft), a new unit version (draft) is created for the unit. - Then, if the unit is published all units referencing the component are published as well. - """ - - def test_unit_version_becomes_draft_again(self): - """Test a unit version becomes a draft again. - - Expected results: - 1. The frozen list is None after the unit version becomes a draft again. + 4. The entity list is different from the previous version. """