Skip to content

Consolidate defined_list/initial_list/frozen_list into entity_list #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bradenmacdonald merged 1 commit into from
Feb 21, 2025
Merged

Consolidate defined_list/initial_list/frozen_list into entity_list #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 13 additions & 148 deletions openedx_learning/apps/authoring/containers/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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?
Expand All @@ -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",
Expand Down
4 changes: 1 addition & 3 deletions openedx_learning/apps/authoring/containers/migrations/0001_initial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
46 changes: 5 additions & 41 deletions openedx_learning/apps/authoring/containers/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""


Expand Down Expand Up @@ -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.
"""

Expand All @@ -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",
)
2 changes: 1 addition & 1 deletion openedx_learning/apps/authoring/units/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading