From 0d233fda9e1c135ed4c0b709b7128caf089dea20 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 3 Mar 2025 16:16:45 -0800 Subject: [PATCH 1/3] refactor: move 'containers' into 'publishing' app --- openedx_learning/api/authoring.py | 1 - openedx_learning/api/authoring_models.py | 1 - .../apps/authoring/containers/__init__.py | 0 .../apps/authoring/containers/api.py | 451 ------------------ .../apps/authoring/containers/apps.py | 25 - .../containers/migrations/__init__.py | 0 .../apps/authoring/containers/models.py | 120 ----- .../apps/authoring/containers/models_mixin.py | 88 ---- .../apps/authoring/publishing/api.py | 444 ++++++++++++++++- .../apps/authoring/publishing/apps.py | 9 + .../migrations/0003_containers.py} | 10 +- .../apps/authoring/publishing/model_mixins.py | 80 +++- .../apps/authoring/publishing/models.py | 114 +++++ openedx_learning/apps/authoring/units/api.py | 17 +- .../units/migrations/0001_initial.py | 7 +- .../apps/authoring/units/models.py | 2 +- projects/dev.py | 1 - test_settings.py | 1 - 18 files changed, 657 insertions(+), 714 deletions(-) delete mode 100644 openedx_learning/apps/authoring/containers/__init__.py delete mode 100644 openedx_learning/apps/authoring/containers/api.py delete mode 100644 openedx_learning/apps/authoring/containers/apps.py delete mode 100644 openedx_learning/apps/authoring/containers/migrations/__init__.py delete mode 100644 openedx_learning/apps/authoring/containers/models.py delete mode 100644 openedx_learning/apps/authoring/containers/models_mixin.py rename openedx_learning/apps/authoring/{containers/migrations/0001_initial.py => publishing/migrations/0003_containers.py} (89%) diff --git a/openedx_learning/api/authoring.py b/openedx_learning/api/authoring.py index de756616c..c06104878 100644 --- a/openedx_learning/api/authoring.py +++ b/openedx_learning/api/authoring.py @@ -11,7 +11,6 @@ # pylint: disable=wildcard-import from ..apps.authoring.collections.api import * from ..apps.authoring.components.api import * -from ..apps.authoring.containers.api import * from ..apps.authoring.contents.api import * from ..apps.authoring.publishing.api import * from ..apps.authoring.units.api import * diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py index 8c7752b23..47123c56c 100644 --- a/openedx_learning/api/authoring_models.py +++ b/openedx_learning/api/authoring_models.py @@ -9,7 +9,6 @@ # pylint: disable=wildcard-import from ..apps.authoring.collections.models import * from ..apps.authoring.components.models import * -from ..apps.authoring.containers.models import * from ..apps.authoring.contents.models import * from ..apps.authoring.publishing.model_mixins import * from ..apps.authoring.publishing.models import * diff --git a/openedx_learning/apps/authoring/containers/__init__.py b/openedx_learning/apps/authoring/containers/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/openedx_learning/apps/authoring/containers/api.py b/openedx_learning/apps/authoring/containers/api.py deleted file mode 100644 index 883962902..000000000 --- a/openedx_learning/apps/authoring/containers/api.py +++ /dev/null @@ -1,451 +0,0 @@ -""" -Containers API. - -This module provides a set of functions to interact with the containers -models in the Open edX Learning platform. -""" -from dataclasses import dataclass -from datetime import datetime - -from django.core.exceptions import ValidationError -from django.db.models import QuerySet -from django.db.transaction import atomic - -from openedx_learning.apps.authoring.containers.models_mixin import ContainerMixin - -from ..containers.models import Container, ContainerVersion, EntityList, EntityListRow -from ..publishing import api as publishing_api -from ..publishing.models import PublishableEntity, PublishableEntityVersion - -# πŸ›‘ UNSTABLE: All APIs related to containers are unstable until we've figured -# out our approach to dynamic content (randomized, A/B tests, etc.) -__all__ = [ - "create_container", - "create_container_version", - "create_next_container_version", - "create_container_and_version", - "get_container", - "ContainerEntityListEntry", - "get_entities_in_draft_container", - "get_entities_in_published_container", - "contains_unpublished_changes", - "get_containers_with_entity", -] - - -def create_container( - learning_package_id: int, - key: str, - created: datetime, - created_by: int | None, -) -> Container: - """ - [ πŸ›‘ UNSTABLE ] - Create a new container. - - Args: - learning_package_id: The ID of the learning package that contains the container. - key: The key of the container. - created: The date and time the container was created. - created_by: The ID of the user who created the container - - Returns: - The newly created container. - """ - with atomic(): - publishable_entity = publishing_api.create_publishable_entity( - learning_package_id, key, created, created_by - ) - container = Container.objects.create( - publishable_entity=publishable_entity, - ) - return container - - -def create_entity_list() -> EntityList: - """ - [ πŸ›‘ UNSTABLE ] - Create a new entity list. This is an structure that holds a list of entities - that will be referenced by the container. - - Returns: - The newly created entity list. - """ - return EntityList.objects.create() - - -def create_entity_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_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). - - Returns: - The newly created entity list. - """ - order_nums = range(len(entity_pks)) - with atomic(): - entity_list = create_entity_list() - EntityListRow.objects.bulk_create( - [ - EntityListRow( - entity_list=entity_list, - entity_id=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 - ) - ] - ) - return entity_list - - -def create_container_version( - container_pk: int, - version_num: int, - *, - title: str, - publishable_entities_pks: list[int], - entity_version_pks: list[int | None] | None, - created: datetime, - created_by: int | None, -) -> ContainerVersion: - """ - [ πŸ›‘ UNSTABLE ] - Create a new container version. - - Args: - container_pk: 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. - created: The date and time the container version was created. - created_by: The ID of the user who created the container version. - - Returns: - The newly created container version. - """ - with atomic(): - container = Container.objects.select_related("publishable_entity").get(pk=container_pk) - entity = container.publishable_entity - - # Do a quick check that the given entities are in the right learning package: - if PublishableEntity.objects.filter( - pk__in=publishable_entities_pks, - ).exclude( - learning_package_id=entity.learning_package_id, - ).exists(): - raise ValidationError("Container entities must be from the same learning package.") - - assert title is not None - assert publishable_entities_pks is not None - 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, - ) - publishable_entity_version = publishing_api.create_publishable_entity_version( - entity.pk, - version_num=version_num, - title=title, - created=created, - created_by=created_by, - ) - container_version = ContainerVersion.objects.create( - publishable_entity_version=publishable_entity_version, - container_id=container_pk, - entity_list=entity_list, - ) - - return container_version - - -def create_next_container_version( - container_pk: int, - *, - title: str | None, - publishable_entities_pks: list[int] | None, - entity_version_pks: list[int | None] | None, - created: datetime, - created_by: int | None, -) -> ContainerVersion: - """ - [ πŸ›‘ UNSTABLE ] - Create the next version of a container. A new version of the container is created - only when its metadata changes: - - * Something was added to the Container. - * We re-ordered the rows in the container. - * Something was removed from the container. - * The Container's metadata changed, e.g. the title. - * We pin to different versions of the Container. - - 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. - created: The date and time the container version was created. - created_by: The ID of the user who created the container version. - - Returns: - The newly created container version. - """ - with atomic(): - container = Container.objects.select_related("publishable_entity").get(pk=container_pk) - entity = container.publishable_entity - 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: - # We're only changing metadata. Keep the same entity list. - next_entity_list = last_version.entity_list - else: - # Do a quick check that the given entities are in the right learning package: - if PublishableEntity.objects.filter( - pk__in=publishable_entities_pks, - ).exclude( - learning_package_id=entity.learning_package_id, - ).exists(): - raise ValidationError("Container entities must be from the same learning package.") - if entity_version_pks is None: - entity_version_pks = [None] * len(publishable_entities_pks) - next_entity_list = create_entity_list_with_rows( - entity_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - ) - publishable_entity_version = publishing_api.create_publishable_entity_version( - entity.pk, - version_num=next_version_num, - title=title if title is not None else last_version.title, - created=created, - created_by=created_by, - ) - next_container_version = ContainerVersion.objects.create( - publishable_entity_version=publishable_entity_version, - container_id=container_pk, - entity_list=next_entity_list, - ) - - return next_container_version - - -def create_container_and_version( - learning_package_id: int, - key: str, - *, - created: datetime, - created_by: int | None, - title: str, - publishable_entities_pks: list[int], - entity_version_pks: list[int | None], -) -> tuple[Container, ContainerVersion]: - """ - [ πŸ›‘ UNSTABLE ] - Create a new container and its first version. - - Args: - learning_package_id: The ID of the learning package that contains the container. - key: The key of the container. - created: The date and time the container was created. - created_by: The ID of the user who created the container. - version_num: The version number of the container. - title: The title of the container. - members_pk: The IDs of the members of the container. - - Returns: - The newly created container version. - """ - with atomic(): - container = create_container(learning_package_id, key, created, created_by) - container_version = create_container_version( - container.publishable_entity.pk, - 1, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - return (container, container_version) - - -def get_container(pk: int) -> Container: - """ - [ πŸ›‘ UNSTABLE ] - Get a container by its primary key. - - Args: - pk: The primary key of the container. - - Returns: - The container with the given primary key. - """ - # TODO: should this use with_publishing_relations as in components? - return Container.objects.get(pk=pk) - - -@dataclass(frozen=True) -class ContainerEntityListEntry: - """ - [ πŸ›‘ UNSTABLE ] - Data about a single entity in a container, e.g. a component in a unit. - """ - entity_version: PublishableEntityVersion - pinned: bool - - @property - def entity(self): - return self.entity_version.entity - - -def get_entities_in_draft_container( - container: Container | ContainerMixin, -) -> list[ContainerEntityListEntry]: - """ - [ πŸ›‘ UNSTABLE ] - Get the list of entities and their versions in the draft version of the - given container. - """ - if isinstance(container, ContainerMixin): - container = container.container - assert isinstance(container, Container) - entity_list = [] - 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( - entity_version=row.entity_version or row.entity.draft.version, - pinned=row.entity_version is not None, - )) - # else should we indicate somehow a deleted item was here? - return entity_list - - -def get_entities_in_published_container( - container: Container | ContainerMixin, -) -> list[ContainerEntityListEntry] | None: - """ - [ πŸ›‘ UNSTABLE ] - Get the list of entities and their versions in the draft version of the - given container. - """ - if isinstance(container, ContainerMixin): - cv = container.container.versioning.published - elif isinstance(container, Container): - cv = container.versioning.published - else: - raise TypeError(f"Expected Container or ContainerMixin; got {type(container)}") - if cv is None: - return None # There is no published version of this container. Should this be an exception? - assert isinstance(cv, ContainerVersion) - entity_list = [] - for row in cv.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( - entity_version=entity_version, - pinned=row.entity_version is not None, - )) - # else should we indicate somehow a deleted item was here? - return entity_list - - -def contains_unpublished_changes( - container: Container | ContainerMixin, -) -> bool: - """ - [ πŸ›‘ UNSTABLE ] - Check recursively if a container has any unpublished changes. - - Note: container.versioning.has_unpublished_changes only checks if the container - itself has unpublished changes, not if its contents do. - """ - if isinstance(container, ContainerMixin): - # The query below pre-loads the data we need but is otherwise the same thing as: - # container = container.container - container = Container.objects.select_related( - "publishable_entity", - "publishable_entity__draft", - "publishable_entity__draft__version", - "publishable_entity__draft__version__containerversion__entity_list", - ).get(pk=container.container_id) - else: - pass # TODO: select_related if we're given a raw Container rather than a ContainerMixin like Unit? - assert isinstance(container, Container) - - if container.versioning.has_unpublished_changes: - return True - - # We only care about children that are un-pinned, since published changes to pinned children don't matter - 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 entity_list.entitylistrow_set.filter(entity_version=None).select_related( - "entity__container", - "entity__draft__version", - "entity__published__version", - ): - try: - child_container = row.entity.container - except Container.DoesNotExist: - child_container = None - if child_container: - child_container = row.entity.container - # This is itself a container - check recursively: - if child_container.versioning.has_unpublished_changes or contains_unpublished_changes(child_container): - return True - else: - # This is not a container: - draft_pk = row.entity.draft.version_id if row.entity.draft else None - published_pk = row.entity.published.version_id if hasattr(row.entity, "published") else None - if draft_pk != published_pk: - return True - return False - - -def get_containers_with_entity( - publishable_entity_pk: int, - *, - ignore_pinned=False, -) -> QuerySet[Container]: - """ - [ πŸ›‘ UNSTABLE ] - Find all draft containers that directly contain the given entity. - - They will always be from the same learning package; cross-package containers - are not allowed. - - Args: - publishable_entity_pk: The ID of the PublishableEntity to search for. - ignore_pinned: if true, ignore any pinned references to the entity. - """ - if ignore_pinned: - qs = Container.objects.filter( - publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 - publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_version_id=None, # pylint: disable=line-too-long # noqa: E501 - ).order_by("pk") # Ordering is mostly for consistent test cases. - else: - qs = Container.objects.filter( - publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 - ).order_by("pk") # Ordering is mostly for consistent test cases. - # Could alternately do this query in two steps. Not sure which is more efficient; depends on how the DB plans it. - # # Find all the EntityLists that contain the given entity: - # lists = EntityList.objects.filter(entitylistrow__entity_id=publishable_entity_pk).values_list('pk', flat=True) - # qs = Container.objects.filter( - # publishable_entity__draft__version__containerversion__entity_list__in=lists - # ) - return qs diff --git a/openedx_learning/apps/authoring/containers/apps.py b/openedx_learning/apps/authoring/containers/apps.py deleted file mode 100644 index 331ae4d14..000000000 --- a/openedx_learning/apps/authoring/containers/apps.py +++ /dev/null @@ -1,25 +0,0 @@ -""" -Containers Django application initialization. -""" - -from django.apps import AppConfig - - -class ContainersConfig(AppConfig): - """ - Configuration for the containers Django application. - """ - - name = "openedx_learning.apps.authoring.containers" - verbose_name = "Learning Core > Authoring > Containers" - default_auto_field = "django.db.models.BigAutoField" - label = "oel_containers" - - def ready(self): - """ - Register Container and ContainerVersion. - """ - from ..publishing.api import register_content_models # pylint: disable=import-outside-toplevel - from .models import Container, ContainerVersion # pylint: disable=import-outside-toplevel - - register_content_models(Container, ContainerVersion) diff --git a/openedx_learning/apps/authoring/containers/migrations/__init__.py b/openedx_learning/apps/authoring/containers/migrations/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/openedx_learning/apps/authoring/containers/models.py b/openedx_learning/apps/authoring/containers/models.py deleted file mode 100644 index 4bfa0ecb7..000000000 --- a/openedx_learning/apps/authoring/containers/models.py +++ /dev/null @@ -1,120 +0,0 @@ -""" -Models that implement containers -""" -from django.db import models - -from openedx_learning.apps.authoring.publishing.models import PublishableEntity, PublishableEntityVersion - -from ..publishing.model_mixins import PublishableEntityMixin, PublishableEntityVersionMixin - -__all__ = [ - "Container", - "ContainerVersion", -] - - -class EntityList(models.Model): - """ - EntityLists are a common structure to hold parent-child relations. - - EntityLists are not PublishableEntities in and of themselves. That's because - 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 ContainerVersions and - other models, rather than being looked up by their own identifiers. - """ - - -class EntityListRow(models.Model): - """ - Each EntityListRow points to a PublishableEntity, optionally at a specific - version. - - There is a row in this table for each member of an EntityList. The order_num - field is used to determine the order of the members in the list. - """ - - entity_list = models.ForeignKey(EntityList, on_delete=models.CASCADE) - - # This ordering should be treated as immutable–if the ordering needs to - # change, we create a new EntityList and copy things over. - order_num = models.PositiveIntegerField() - - # Simple case would use these fields with our convention that null versions - # means "get the latest draft or published as appropriate". These entities - # could be Selectors, in which case we'd need to do more work to find the right - # variant. The publishing app itself doesn't know anything about Selectors - # however, and just treats it as another PublishableEntity. - entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) - - # The version references point to the specific PublishableEntityVersion that - # this EntityList has for this PublishableEntity for both the draft and - # published states. However, we don't want to have to create new EntityList - # every time that a member is updated, because that would waste a lot of - # space and make it difficult to figure out when the metadata of something - # like a Unit *actually* changed, vs. when its child members were being - # updated. Doing so could also potentially lead to race conditions when - # updating multiple layers of containers. - # - # So our approach to this is to use a value of None (null) to represent an - # unpinned reference to a PublishableEntity. It's shorthand for "just use - # the latest draft or published version of this, as appropriate". - entity_version = models.ForeignKey( - PublishableEntityVersion, - on_delete=models.RESTRICT, - null=True, - related_name="+", # Do we need the reverse relation? - ) - - -class Container(PublishableEntityMixin): - """ - A Container is a type of PublishableEntity that holds other - PublishableEntities. For example, a "Unit" Container might hold several - Components. - - For now, all containers have a static "entity list" that defines which - containers/components/enities they hold. As we complete the Containers API, - we will also add support for dynamic containers which may contain different - entities for different learners or at different times. - - NOTE: We're going to want to eventually have some association between the - PublishLog and Containers that were affected in a publish because their - child elements were published. - """ - - -class ContainerVersion(PublishableEntityVersionMixin): - """ - A version of a Container. - - By convention, we would only want to create new versions when the Container - itself changes, and not when the Container's child elements change. For - example: - - * Something was added to the Container. - * We re-ordered the rows in the container. - * Something was removed to the container. - * The Container's metadata changed, e.g. the title. - * We pin to different versions of the Container. - - 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 entity_list never - changes for a given ContainerVersion. - """ - - container = models.ForeignKey( - Container, - on_delete=models.CASCADE, - related_name="versions", - ) - - # The list of entities (frozen and/or unfrozen) in this container - entity_list = models.ForeignKey( - EntityList, - on_delete=models.RESTRICT, - null=False, - related_name="entity_list", - ) diff --git a/openedx_learning/apps/authoring/containers/models_mixin.py b/openedx_learning/apps/authoring/containers/models_mixin.py deleted file mode 100644 index 2d76cd252..000000000 --- a/openedx_learning/apps/authoring/containers/models_mixin.py +++ /dev/null @@ -1,88 +0,0 @@ -""" -Mixins for models that implement containers -""" -from __future__ import annotations - -from typing import ClassVar, Self - -from django.db import models - -from openedx_learning.apps.authoring.containers.models import Container, ContainerVersion -from openedx_learning.apps.authoring.publishing.model_mixins import ( - PublishableEntityMixin, - PublishableEntityVersionMixin, -) -from openedx_learning.lib.managers import WithRelationsManager - -__all__ = [ - "ContainerMixin", - "ContainerVersionMixin", -] - - -class ContainerMixin(PublishableEntityMixin): - """ - Convenience mixin to link your models against Container. - - Please see docstring for Container for more details. - - If you use this class, you *MUST* also use ContainerVersionMixin - """ - - # select these related entities by default for all queries - objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager("container") # type: ignore[assignment] - - container = models.OneToOneField( - Container, - on_delete=models.CASCADE, - ) - - @property - def uuid(self): - return self.container.uuid - - @property - def created(self): - return self.container.created - - class Meta: - abstract = True - - -class ContainerVersionMixin(PublishableEntityVersionMixin): - """ - Convenience mixin to link your models against ContainerVersion. - - Please see docstring for ContainerVersion for more details. - - If you use this class, you *MUST* also use ContainerMixin - """ - - # select these related entities by default for all queries - objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] - "container_version", - ) - - container_version = models.OneToOneField( - ContainerVersion, - on_delete=models.CASCADE, - ) - - @property - def uuid(self): - return self.container_version.uuid - - @property - def title(self): - return self.container_version.title - - @property - def created(self): - return self.container_version.created - - @property - def version_num(self): - return self.container_version.version_num - - class Meta: - abstract = True diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 1a61ed70c..fe0c5f5f5 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -6,15 +6,25 @@ """ from __future__ import annotations +from dataclasses import dataclass from datetime import datetime, timezone -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet from django.db.transaction import atomic -from .model_mixins import PublishableContentModelRegistry, PublishableEntityMixin, PublishableEntityVersionMixin +from .model_mixins import ( + ContainerMixin, + PublishableContentModelRegistry, + PublishableEntityMixin, + PublishableEntityVersionMixin, +) from .models import ( + Container, + ContainerVersion, Draft, + EntityList, + EntityListRow, LearningPackage, PublishableEntity, PublishableEntityVersion, @@ -51,6 +61,18 @@ "reset_drafts_to_published", "register_content_models", "filter_publishable_entities", + # πŸ›‘ UNSTABLE: All APIs related to containers are unstable until we've figured + # out our approach to dynamic content (randomized, A/B tests, etc.) + "create_container", + "create_container_version", + "create_next_container_version", + "create_container_and_version", + "get_container", + "ContainerEntityListEntry", + "get_entities_in_draft_container", + "get_entities_in_published_container", + "contains_unpublished_changes", + "get_containers_with_entity", ] @@ -528,3 +550,421 @@ def get_published_version_as_of(entity_id: int, publish_log_id: int) -> Publisha publish_log_id__lte=publish_log_id, ).order_by('-publish_log_id').first() return record.new_version if record else None + + +def create_container( + learning_package_id: int, + key: str, + created: datetime, + created_by: int | None, +) -> Container: + """ + [ πŸ›‘ UNSTABLE ] + Create a new container. + + Args: + learning_package_id: The ID of the learning package that contains the container. + key: The key of the container. + created: The date and time the container was created. + created_by: The ID of the user who created the container + + Returns: + The newly created container. + """ + with atomic(): + publishable_entity = create_publishable_entity( + learning_package_id, key, created, created_by + ) + container = Container.objects.create( + publishable_entity=publishable_entity, + ) + return container + + +def create_entity_list() -> EntityList: + """ + [ πŸ›‘ UNSTABLE ] + Create a new entity list. This is an structure that holds a list of entities + that will be referenced by the container. + + Returns: + The newly created entity list. + """ + return EntityList.objects.create() + + +def create_entity_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_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). + + Returns: + The newly created entity list. + """ + order_nums = range(len(entity_pks)) + with atomic(): + entity_list = create_entity_list() + EntityListRow.objects.bulk_create( + [ + EntityListRow( + entity_list=entity_list, + entity_id=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 + ) + ] + ) + return entity_list + + +def create_container_version( + container_pk: int, + version_num: int, + *, + title: str, + publishable_entities_pks: list[int], + entity_version_pks: list[int | None] | None, + created: datetime, + created_by: int | None, +) -> ContainerVersion: + """ + [ πŸ›‘ UNSTABLE ] + Create a new container version. + + Args: + container_pk: 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. + created: The date and time the container version was created. + created_by: The ID of the user who created the container version. + + Returns: + The newly created container version. + """ + with atomic(): + container = Container.objects.select_related("publishable_entity").get(pk=container_pk) + entity = container.publishable_entity + + # Do a quick check that the given entities are in the right learning package: + if PublishableEntity.objects.filter( + pk__in=publishable_entities_pks, + ).exclude( + learning_package_id=entity.learning_package_id, + ).exists(): + raise ValidationError("Container entities must be from the same learning package.") + + assert title is not None + assert publishable_entities_pks is not None + 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, + ) + publishable_entity_version = create_publishable_entity_version( + entity.pk, + version_num=version_num, + title=title, + created=created, + created_by=created_by, + ) + container_version = ContainerVersion.objects.create( + publishable_entity_version=publishable_entity_version, + container_id=container_pk, + entity_list=entity_list, + ) + + return container_version + + +def create_next_container_version( + container_pk: int, + *, + title: str | None, + publishable_entities_pks: list[int] | None, + entity_version_pks: list[int | None] | None, + created: datetime, + created_by: int | None, +) -> ContainerVersion: + """ + [ πŸ›‘ UNSTABLE ] + Create the next version of a container. A new version of the container is created + only when its metadata changes: + + * Something was added to the Container. + * We re-ordered the rows in the container. + * Something was removed from the container. + * The Container's metadata changed, e.g. the title. + * We pin to different versions of the Container. + + 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. + created: The date and time the container version was created. + created_by: The ID of the user who created the container version. + + Returns: + The newly created container version. + """ + with atomic(): + container = Container.objects.select_related("publishable_entity").get(pk=container_pk) + entity = container.publishable_entity + 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: + # We're only changing metadata. Keep the same entity list. + next_entity_list = last_version.entity_list + else: + # Do a quick check that the given entities are in the right learning package: + if PublishableEntity.objects.filter( + pk__in=publishable_entities_pks, + ).exclude( + learning_package_id=entity.learning_package_id, + ).exists(): + raise ValidationError("Container entities must be from the same learning package.") + if entity_version_pks is None: + entity_version_pks = [None] * len(publishable_entities_pks) + next_entity_list = create_entity_list_with_rows( + entity_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + ) + publishable_entity_version = create_publishable_entity_version( + entity.pk, + version_num=next_version_num, + title=title if title is not None else last_version.title, + created=created, + created_by=created_by, + ) + next_container_version = ContainerVersion.objects.create( + publishable_entity_version=publishable_entity_version, + container_id=container_pk, + entity_list=next_entity_list, + ) + + return next_container_version + + +def create_container_and_version( + learning_package_id: int, + key: str, + *, + created: datetime, + created_by: int | None, + title: str, + publishable_entities_pks: list[int], + entity_version_pks: list[int | None], +) -> tuple[Container, ContainerVersion]: + """ + [ πŸ›‘ UNSTABLE ] + Create a new container and its first version. + + Args: + learning_package_id: The ID of the learning package that contains the container. + key: The key of the container. + created: The date and time the container was created. + created_by: The ID of the user who created the container. + version_num: The version number of the container. + title: The title of the container. + members_pk: The IDs of the members of the container. + + Returns: + The newly created container version. + """ + with atomic(): + container = create_container(learning_package_id, key, created, created_by) + container_version = create_container_version( + container.publishable_entity.pk, + 1, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + ) + return (container, container_version) + + +def get_container(pk: int) -> Container: + """ + [ πŸ›‘ UNSTABLE ] + Get a container by its primary key. + + Args: + pk: The primary key of the container. + + Returns: + The container with the given primary key. + """ + # TODO: should this use with_publishing_relations as in components? + return Container.objects.get(pk=pk) + + +@dataclass(frozen=True) +class ContainerEntityListEntry: + """ + [ πŸ›‘ UNSTABLE ] + Data about a single entity in a container, e.g. a component in a unit. + """ + entity_version: PublishableEntityVersion + pinned: bool + + @property + def entity(self): + return self.entity_version.entity + + +def get_entities_in_draft_container( + container: Container | ContainerMixin, +) -> list[ContainerEntityListEntry]: + """ + [ πŸ›‘ UNSTABLE ] + Get the list of entities and their versions in the draft version of the + given container. + """ + if isinstance(container, ContainerMixin): + container = container.container + assert isinstance(container, Container) + entity_list = [] + 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( + entity_version=row.entity_version or row.entity.draft.version, + pinned=row.entity_version is not None, + )) + # else should we indicate somehow a deleted item was here? + return entity_list + + +def get_entities_in_published_container( + container: Container | ContainerMixin, +) -> list[ContainerEntityListEntry] | None: + """ + [ πŸ›‘ UNSTABLE ] + Get the list of entities and their versions in the draft version of the + given container. + """ + if isinstance(container, ContainerMixin): + cv = container.container.versioning.published + elif isinstance(container, Container): + cv = container.versioning.published + else: + raise TypeError(f"Expected Container or ContainerMixin; got {type(container)}") + if cv is None: + return None # There is no published version of this container. Should this be an exception? + assert isinstance(cv, ContainerVersion) + entity_list = [] + for row in cv.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( + entity_version=entity_version, + pinned=row.entity_version is not None, + )) + # else should we indicate somehow a deleted item was here? + return entity_list + + +def contains_unpublished_changes( + container: Container | ContainerMixin, +) -> bool: + """ + [ πŸ›‘ UNSTABLE ] + Check recursively if a container has any unpublished changes. + + Note: container.versioning.has_unpublished_changes only checks if the container + itself has unpublished changes, not if its contents do. + """ + if isinstance(container, ContainerMixin): + # The query below pre-loads the data we need but is otherwise the same thing as: + # container = container.container + container = Container.objects.select_related( + "publishable_entity", + "publishable_entity__draft", + "publishable_entity__draft__version", + "publishable_entity__draft__version__containerversion__entity_list", + ).get(pk=container.container_id) + else: + pass # TODO: select_related if we're given a raw Container rather than a ContainerMixin like Unit? + assert isinstance(container, Container) + + if container.versioning.has_unpublished_changes: + return True + + # We only care about children that are un-pinned, since published changes to pinned children don't matter + 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 entity_list.entitylistrow_set.filter(entity_version=None).select_related( + "entity__container", + "entity__draft__version", + "entity__published__version", + ): + try: + child_container = row.entity.container + except Container.DoesNotExist: + child_container = None + if child_container: + child_container = row.entity.container + # This is itself a container - check recursively: + if child_container.versioning.has_unpublished_changes or contains_unpublished_changes(child_container): + return True + else: + # This is not a container: + draft_pk = row.entity.draft.version_id if row.entity.draft else None + published_pk = row.entity.published.version_id if hasattr(row.entity, "published") else None + if draft_pk != published_pk: + return True + return False + + +def get_containers_with_entity( + publishable_entity_pk: int, + *, + ignore_pinned=False, +) -> QuerySet[Container]: + """ + [ πŸ›‘ UNSTABLE ] + Find all draft containers that directly contain the given entity. + + They will always be from the same learning package; cross-package containers + are not allowed. + + Args: + publishable_entity_pk: The ID of the PublishableEntity to search for. + ignore_pinned: if true, ignore any pinned references to the entity. + """ + if ignore_pinned: + qs = Container.objects.filter( + publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 + publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_version_id=None, # pylint: disable=line-too-long # noqa: E501 + ).order_by("pk") # Ordering is mostly for consistent test cases. + else: + qs = Container.objects.filter( + publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 + ).order_by("pk") # Ordering is mostly for consistent test cases. + # Could alternately do this query in two steps. Not sure which is more efficient; depends on how the DB plans it. + # # Find all the EntityLists that contain the given entity: + # lists = EntityList.objects.filter(entitylistrow__entity_id=publishable_entity_pk).values_list('pk', flat=True) + # qs = Container.objects.filter( + # publishable_entity__draft__version__containerversion__entity_list__in=lists + # ) + return qs diff --git a/openedx_learning/apps/authoring/publishing/apps.py b/openedx_learning/apps/authoring/publishing/apps.py index bfa1bbe62..f5a8b1f9a 100644 --- a/openedx_learning/apps/authoring/publishing/apps.py +++ b/openedx_learning/apps/authoring/publishing/apps.py @@ -14,3 +14,12 @@ class PublishingConfig(AppConfig): verbose_name = "Learning Core > Authoring > Publishing" default_auto_field = "django.db.models.BigAutoField" label = "oel_publishing" + + def ready(self): + """ + Register Container and ContainerVersion. + """ + from .api import register_content_models # pylint: disable=import-outside-toplevel + from .models import Container, ContainerVersion # pylint: disable=import-outside-toplevel + + register_content_models(Container, ContainerVersion) diff --git a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py similarity index 89% rename from openedx_learning/apps/authoring/containers/migrations/0001_initial.py rename to openedx_learning/apps/authoring/publishing/migrations/0003_containers.py index 90be54f9c..70babb328 100644 --- a/openedx_learning/apps/authoring/containers/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.19 on 2025-02-14 23:04 +# Generated by Django 4.2.19 on 2025-03-04 00:02 import django.db.models.deletion from django.db import migrations, models @@ -6,8 +6,6 @@ class Migration(migrations.Migration): - initial = True - dependencies = [ ('oel_publishing', '0002_alter_learningpackage_key_and_more'), ] @@ -34,7 +32,7 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('order_num', models.PositiveIntegerField()), ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), - ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.entitylist')), + ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.entitylist')), ('entity_version', models.ForeignKey(null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')), ], ), @@ -42,8 +40,8 @@ class Migration(migrations.Migration): name='ContainerVersion', 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.container')), - ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='entity_list', to='oel_containers.entitylist')), + ('container', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_publishing.container')), + ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='entity_list', to='oel_publishing.entitylist')), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/publishing/model_mixins.py b/openedx_learning/apps/authoring/publishing/model_mixins.py index 8743b333a..7fa9136b2 100644 --- a/openedx_learning/apps/authoring/publishing/model_mixins.py +++ b/openedx_learning/apps/authoring/publishing/model_mixins.py @@ -3,20 +3,24 @@ """ from __future__ import annotations +from datetime import datetime from functools import cached_property -from typing import ClassVar, Self +from typing import ClassVar, Self, TYPE_CHECKING from django.core.exceptions import ImproperlyConfigured from django.db import models from openedx_learning.lib.managers import WithRelationsManager -from .models import PublishableEntity, PublishableEntityVersion +if TYPE_CHECKING: + from .models import PublishableEntityVersion __all__ = [ "PublishableEntityMixin", "PublishableEntityVersionMixin", "PublishableContentModelRegistry", + "ContainerMixin", + "ContainerVersionMixin", ] @@ -38,7 +42,7 @@ class PublishableEntityMixin(models.Model): ) publishable_entity = models.OneToOneField( - PublishableEntity, on_delete=models.CASCADE, primary_key=True + "oel_publishing.PublishableEntity", on_delete=models.CASCADE, primary_key=True ) @cached_property @@ -297,7 +301,7 @@ class PublishableEntityVersionMixin(models.Model): ) publishable_entity_version = models.OneToOneField( - PublishableEntityVersion, on_delete=models.CASCADE, primary_key=True + "oel_publishing.PublishableEntityVersion", on_delete=models.CASCADE, primary_key=True ) @property @@ -360,3 +364,71 @@ def get_versioned_model_cls(cls, content_model_cls): @classmethod def get_unversioned_model_cls(cls, content_version_model_cls): return cls._versioned_to_unversioned[content_version_model_cls] + + +class ContainerMixin(PublishableEntityMixin): + """ + Convenience mixin to link your models against Container. + + Please see docstring for Container for more details. + + If you use this class, you *MUST* also use ContainerVersionMixin + """ + + # select these related entities by default for all queries + objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager("container") # type: ignore[assignment] + + container = models.OneToOneField( + "oel_publishing.Container", + on_delete=models.CASCADE, + ) + + @property + def uuid(self) -> str: + return self.container.uuid + + @property + def created(self) -> datetime: + return self.container.created + + class Meta: + abstract = True + + +class ContainerVersionMixin(PublishableEntityVersionMixin): + """ + Convenience mixin to link your models against ContainerVersion. + + Please see docstring for ContainerVersion for more details. + + If you use this class, you *MUST* also use ContainerMixin + """ + + # select these related entities by default for all queries + objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] + "container_version", + ) + + container_version = models.OneToOneField( + "oel_publishing.ContainerVersion", + on_delete=models.CASCADE, + ) + + @property + def uuid(self) -> str: + return self.container_version.uuid + + @property + def title(self) -> str: + return self.container_version.title + + @property + def created(self) -> datetime: + return self.container_version.created + + @property + def version_num(self) -> int: + return self.container_version.version_num + + class Meta: + abstract = True diff --git a/openedx_learning/apps/authoring/publishing/models.py b/openedx_learning/apps/authoring/publishing/models.py index d62ba012f..142321035 100644 --- a/openedx_learning/apps/authoring/publishing/models.py +++ b/openedx_learning/apps/authoring/publishing/models.py @@ -23,6 +23,8 @@ manual_date_time_field, ) +from .model_mixins import PublishableEntityMixin, PublishableEntityVersionMixin + __all__ = [ "LearningPackage", "PublishableEntity", @@ -31,6 +33,10 @@ "PublishLog", "PublishLogRecord", "Published", + "EntityList", + "EntityListRow", + "Container", + "ContainerVersion" ] @@ -515,3 +521,111 @@ class Published(models.Model): class Meta: verbose_name = "Published Entity" verbose_name_plural = "Published Entities" + + +class EntityList(models.Model): + """ + An EntityList is a static list of PublishableEntities, usually used to model + parent-child relationships. + + EntityLists are not PublishableEntities in and of themselves. That's because + 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 ContainerVersions and + other models, rather than being looked up by their own identifiers. + """ + + +class EntityListRow(models.Model): + """ + Each EntityListRow points to a PublishableEntity, optionally at a specific + version. + + There is a row in this table for each member of an EntityList. The order_num + field is used to determine the order of the members in the list. + """ + + entity_list = models.ForeignKey(EntityList, on_delete=models.CASCADE) + + # This ordering should be treated as immutable–if the ordering needs to + # change, we create a new EntityList and copy things over. + order_num = models.PositiveIntegerField() + + # Simple case would use these fields with our convention that null versions + # means "get the latest draft or published as appropriate". These entities + # could be Selectors, in which case we'd need to do more work to find the right + # variant. The publishing app itself doesn't know anything about Selectors + # however, and just treats it as another PublishableEntity. + entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) + + # The version references point to the specific PublishableEntityVersion that + # this EntityList has for this PublishableEntity for both the draft and + # published states. However, we don't want to have to create new EntityList + # every time that a member is updated, because that would waste a lot of + # space and make it difficult to figure out when the metadata of something + # like a Unit *actually* changed, vs. when its child members were being + # updated. Doing so could also potentially lead to race conditions when + # updating multiple layers of containers. + # + # So our approach to this is to use a value of None (null) to represent an + # unpinned reference to a PublishableEntity. It's shorthand for "just use + # the latest draft or published version of this, as appropriate". + entity_version = models.ForeignKey( + PublishableEntityVersion, + on_delete=models.RESTRICT, + null=True, + related_name="+", # Do we need the reverse relation? + ) + + +class Container(PublishableEntityMixin): + """ + A Container is a type of PublishableEntity that holds other + PublishableEntities. For example, a "Unit" Container might hold several + Components. + + For now, all containers have a static "entity list" that defines which + containers/components/enities they hold. As we complete the Containers API, + we will also add support for dynamic containers which may contain different + entities for different learners or at different times. + + NOTE: We're going to want to eventually have some association between the + PublishLog and Containers that were affected in a publish because their + child elements were published. + """ + + +class ContainerVersion(PublishableEntityVersionMixin): + """ + A version of a Container. + + By convention, we would only want to create new versions when the Container + itself changes, and not when the Container's child elements change. For + example: + + * Something was added to the Container. + * We re-ordered the rows in the container. + * Something was removed to the container. + * The Container's metadata changed, e.g. the title. + * We pin to different versions of the Container. + + 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 entity_list never + changes for a given ContainerVersion. + """ + + container = models.ForeignKey( + Container, + on_delete=models.CASCADE, + related_name="versions", + ) + + # The list of entities (frozen and/or unfrozen) in this container + entity_list = models.ForeignKey( + EntityList, + on_delete=models.RESTRICT, + null=False, + related_name="entity_list", + ) diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 9da2703ad..e87875158 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -9,8 +9,7 @@ from openedx_learning.apps.authoring.components.models import Component, ComponentVersion -from ..containers import api as container_api -from ..publishing.api import get_published_version_as_of +from ..publishing import api as publishing_api from .models import Unit, UnitVersion # πŸ›‘ UNSTABLE: All APIs related to containers are unstable until we've figured @@ -43,7 +42,7 @@ def create_unit( created_by: The user who created the unit. """ with atomic(): - container = container_api.create_container( + container = publishing_api.create_container( learning_package_id, key, created, created_by ) unit = Unit.objects.create( @@ -76,7 +75,7 @@ def create_unit_version( created_by: The user who created the unit. """ with atomic(): - container_version = container_api.create_container_version( + container_version = publishing_api.create_container_version( unit.container.pk, version_num, title=title, @@ -129,7 +128,7 @@ def create_next_unit_version( publishable_entities_pks = None entity_version_pks = None with atomic(): - container_version = container_api.create_next_container_version( + container_version = publishing_api.create_next_container_version( unit.container.pk, title=title, publishable_entities_pks=publishable_entities_pks, @@ -229,7 +228,7 @@ def get_components_in_draft_unit( """ assert isinstance(unit, Unit) entity_list = [] - for entry in container_api.get_entities_in_draft_container(unit): + for entry in publishing_api.get_entities_in_draft_container(unit): # Convert from generic PublishableEntityVersion to ComponentVersion: component_version = entry.entity_version.componentversion assert isinstance(component_version, ComponentVersion) @@ -248,7 +247,7 @@ def get_components_in_published_unit( Returns None if the unit was never published (TODO: should it throw instead?). """ assert isinstance(unit, Unit) - published_entities = container_api.get_entities_in_published_container(unit) + published_entities = publishing_api.get_entities_in_published_container(unit) if published_entities is None: return None # There is no published version of this unit. Should this be an exception? entity_list = [] @@ -279,7 +278,7 @@ def get_components_in_published_unit_as_of( ancestors of every modified PublishableEntity in the publish. """ assert isinstance(unit, Unit) - unit_pub_entity_version = get_published_version_as_of(unit.publishable_entity_id, publish_log_id) + unit_pub_entity_version = publishing_api.get_published_version_as_of(unit.publishable_entity_id, publish_log_id) if unit_pub_entity_version is None: return None # This unit was not published as of the given PublishLog ID. unit_version = unit_pub_entity_version.unitversion # type: ignore[attr-defined] @@ -294,7 +293,7 @@ def get_components_in_published_unit_as_of( else: # Unpinned component - figure out what its latest published version was. # This is not optimized. It could be done in one query per unit rather than one query per component. - pub_entity_version = get_published_version_as_of(row.entity_id, publish_log_id) + pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id) if pub_entity_version: entity_list.append(UnitListEntry(component_version=pub_entity_version.componentversion, pinned=False)) return entity_list diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py index 8a72507e4..85303617b 100644 --- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py @@ -9,8 +9,7 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_containers', '0001_initial'), - ('oel_publishing', '0002_alter_learningpackage_key_and_more'), + ('oel_publishing', '0003_containers'), ] operations = [ @@ -18,7 +17,7 @@ class Migration(migrations.Migration): name='Unit', fields=[ ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.container')), + ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.container')), ], options={ 'abstract': False, @@ -28,7 +27,7 @@ class Migration(migrations.Migration): name='UnitVersion', fields=[ ('publishable_entity_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentityversion')), - ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.containerversion')), + ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.containerversion')), ('unit', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_units.unit')), ], options={ diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 3e5044916..3a12e035c 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -3,7 +3,7 @@ """ from django.db import models -from ..containers.models_mixin import ContainerMixin, ContainerVersionMixin +from ..publishing.model_mixins import ContainerMixin, ContainerVersionMixin __all__ = [ "Unit", diff --git a/projects/dev.py b/projects/dev.py index 094494abc..85d01f4a5 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -35,7 +35,6 @@ "openedx_learning.apps.authoring.components.apps.ComponentsConfig", "openedx_learning.apps.authoring.contents.apps.ContentsConfig", "openedx_learning.apps.authoring.publishing.apps.PublishingConfig", - "openedx_learning.apps.authoring.containers.apps.ContainersConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", # Learning Contrib Apps "openedx_learning.contrib.media_server.apps.MediaServerConfig", diff --git a/test_settings.py b/test_settings.py index 9b58f9093..71a71c33e 100644 --- a/test_settings.py +++ b/test_settings.py @@ -45,7 +45,6 @@ def root(*args): "openedx_learning.apps.authoring.contents.apps.ContentsConfig", "openedx_learning.apps.authoring.publishing.apps.PublishingConfig", "openedx_tagging.core.tagging.apps.TaggingConfig", - "openedx_learning.apps.authoring.containers.apps.ContainersConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", ] From 64bd03d436b2ce6be81840427057107f15232d51 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 3 Mar 2025 16:16:56 -0800 Subject: [PATCH 2/3] chore: fix some type issues --- .../apps/authoring/components/api.py | 2 +- .../apps/authoring/publishing/model_mixins.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 7f5bb33e5..1c6d1693e 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -468,7 +468,7 @@ def _get_component_version_info_headers(component_version: ComponentVersion) -> "X-Open-edX-Component-Uuid": component.uuid, # Component Version "X-Open-edX-Component-Version-Uuid": component_version.uuid, - "X-Open-edX-Component-Version-Num": component_version.version_num, + "X-Open-edX-Component-Version-Num": str(component_version.version_num), # Learning Package "X-Open-edX-Learning-Package-Key": learning_package.key, "X-Open-edX-Learning-Package-Uuid": learning_package.uuid, diff --git a/openedx_learning/apps/authoring/publishing/model_mixins.py b/openedx_learning/apps/authoring/publishing/model_mixins.py index 7fa9136b2..2a93e6a28 100644 --- a/openedx_learning/apps/authoring/publishing/model_mixins.py +++ b/openedx_learning/apps/authoring/publishing/model_mixins.py @@ -5,7 +5,7 @@ from datetime import datetime from functools import cached_property -from typing import ClassVar, Self, TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar, Self from django.core.exceptions import ImproperlyConfigured from django.db import models @@ -50,15 +50,15 @@ def versioning(self): return self.VersioningHelper(self) @property - def uuid(self): + def uuid(self) -> str: return self.publishable_entity.uuid @property - def key(self): + def key(self) -> str: return self.publishable_entity.key @property - def created(self): + def created(self) -> datetime: return self.publishable_entity.created @property @@ -305,19 +305,19 @@ class PublishableEntityVersionMixin(models.Model): ) @property - def uuid(self): + def uuid(self) -> str: return self.publishable_entity_version.uuid @property - def title(self): + def title(self) -> str: return self.publishable_entity_version.title @property - def created(self): + def created(self) -> datetime: return self.publishable_entity_version.created @property - def version_num(self): + def version_num(self) -> int: return self.publishable_entity_version.version_num class Meta: From 5e782230a9f0890aca81a9d0610ab75ce12e04e6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 4 Mar 2025 18:05:40 -0800 Subject: [PATCH 3/3] feat: auto-publish children when publishing a container --- .../apps/authoring/publishing/api.py | 24 +++++++++++++++++++ .../publishing/migrations/0003_containers.py | 2 +- .../apps/authoring/publishing/models.py | 2 +- .../apps/authoring/units/test_api.py | 10 ++------ 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index fe0c5f5f5..ea8056633 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -332,6 +332,30 @@ def publish_from_drafts( published_at = datetime.now(tz=timezone.utc) with atomic(): + # If the drafts include any containers, we need to auto-publish their descendants: + # TODO: this only handles one level deep and would need to be updated to support sections > subsections > units + + # Get the IDs of the ContainerVersion for any Containers whose drafts are slated to be published. + container_version_ids = ( + Container.objects.filter(publishable_entity__draft__in=draft_qset) + .values_list("publishable_entity__draft__version__containerversion__pk", flat=True) + ) + if container_version_ids: + # We are publishing at least one container. Check if it has any child components that aren't already slated + # to be published. + unpublished_draft_children = EntityListRow.objects.filter( + entity_list__container_versions__pk__in=container_version_ids, + entity_version=None, # Unpinned entities only + ).exclude( + entity__draft__version=F("entity__published__version") # Exclude already published things + ).values_list("entity__draft__pk", flat=True) + if unpublished_draft_children: + # Force these additional child components to be published at the same time by adding them to the qset: + draft_qset = Draft.objects.filter( + Q(pk__in=draft_qset.values_list("pk", flat=True)) | + Q(pk__in=unpublished_draft_children) + ) + # One PublishLog for this entire publish operation. publish_log = PublishLog( learning_package_id=learning_package_id, diff --git a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py index 70babb328..9713e8423 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py @@ -41,7 +41,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_publishing.container')), - ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='entity_list', to='oel_publishing.entitylist')), + ('entity_list', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='container_versions', to='oel_publishing.entitylist')), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/publishing/models.py b/openedx_learning/apps/authoring/publishing/models.py index 142321035..b6e9bd6ca 100644 --- a/openedx_learning/apps/authoring/publishing/models.py +++ b/openedx_learning/apps/authoring/publishing/models.py @@ -627,5 +627,5 @@ class ContainerVersion(PublishableEntityVersionMixin): EntityList, on_delete=models.RESTRICT, null=False, - related_name="entity_list", + related_name="container_versions", ) diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index e7d470d02..5b2bc32f4 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -234,7 +234,6 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self): ] assert authoring_api.get_components_in_published_unit(unit) is None - @pytest.mark.skip(reason="FIXME: auto-publishing children is not implemented yet") def test_auto_publish_children(self): """ Test that publishing a unit publishes its child components automatically. @@ -486,16 +485,11 @@ def test_publishing_shared_component(self): c5_v2 = self.modify_component(c5, title="C5 version 2") # 4️⃣ The author then publishes Unit 1, and therefore everything in it. - # FIXME: this should only require publishing the unit itself, but we don't yet do auto-publishing children authoring_api.publish_from_drafts( self.learning_package.pk, draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter( - entity_id__in=[ - unit1.publishable_entity.id, - c1.publishable_entity.id, - c2.publishable_entity.id, - c3.publishable_entity.id, - ], + # Note: we only publish the unit; the publishing API should auto-publish its components too. + entity_id=unit1.publishable_entity.id, ), )