From c81c94e1e3025ff913d15cc3afa155d651ff904c Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 28 Apr 2026 10:49:13 -0400 Subject: [PATCH 1/3] feat!: Consistently attribute changes with time and author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This modifies the signatures of several API functions in openedx_content such that it is easier to correctly attribute operations to their author and datetime of change, and more difficult to forget to attribute them. BREAKING CHANGE: The `with bulk_draft_changes_for(...):` context manager is renamed `with draft_changes_for(...):`. Furthermore, to encourage callers to correctly associate DraftChangeLogs with their author users whenever possible, the `changed_by=` param is now required (although callers can explicitly pass `changed_by=None` for operations which are truly author-less, like data backfills). Finally, the `changed_at=` param remains optional, but its default value has changed from `None` to `DATETIME_AUTO` and `None` is forbidden; this is just a signature change—as was the default behavior before, `DATETIME_AUTO` will resolve to the current time. This reflects its expanded role: BREAKING CHANGE: Functions which generate DraftChanges may now *only* occur within a `draft_changes_for` context, even if they are single operations (hence the dropping of the `bulk_` prefix from the context manager's name). Furthermore, these functions no longer accept authorship params (`created_by=`, `changed_by=`, etc.) nor datetime params (`created=`, `created_at=`, `changed=`, `changed_at=`, etc.). Instead, they will inherit these values from their enclosing `draft_changes_for(...)` context. The affected functions are: * create_publishable_entity_version * reset_drafts_to_published * soft_delete_draft * set_draft_version * create_(component|container)_version * create_(component|container)_and_version * create_next_(component|container)_version * create_(unit|subsection|section)_and_version * create_next_(unit|subsection|section)_version BREAKING CHANGE: Functions which create or edit versionless content objects (i.e., those which don't generate DraftChanges) may continue to be used inside or outside of `draft_changes_for` contexts, but the default behavior of each function's authorship param (`created_by=`, `changed_by`, etc.) has changed. Previously, it would default to `None` (no author attribution). Now, it defaults to a new sentinel value, `AUTHOR_AUTO`. Within a context, `AUTHOR_AUTO` resolves to the author of the enclosing context. Outside of a context, an exception is raised unless the default `AUTHOR_AUTO` param is specified as something else—either a specific user, or an explicit `None` to indicate a special author-less operation such as a data backfill. Finally, the default of these operations' datetime params (`created=`, `created_at=`, `changed_at`, etc.) is now `DATETIME_AUTO`, paralleling `draft_changes_for(...)`. The affected functions are: * create_publishable_entity * create_(component|container) * create_collection * set_collections BREAKING CHANGE: A couple other functions with `created` params now default to `DATETIME_AUTO` and forbid `None`. They are: * create_learning_package * update_learning_package BREAKING CHANGE: `publish_all_drafts(...)` and `publish_from_drafts(...)` now require the caller to specify `published_by` (a User, user ID, AnonymousUser, or explicit `None`). To match the rest of this rework, `published_at=` now defaults to `DATETIME_AUTO` (resolving to the current time when called outside a `draft_changes_for` context, which is required for these functions). Additionally, `message=`, `published_at=`, and `publish_dependencies=` are now keyword-only. BREAKING CHANGE: `set_collections(...)` now accepts a `created=` timestamp (stored on the m2m through-table along with `created_by`). Both `created=` and `created_by=` are keyword-only and follow the `AUTHOR_AUTO`/`DATETIME_AUTO` defaults described above. Part of: https://github.com/openedx/openedx-core/issues/549 --- .../applets/backup_restore/zipper.py | 8 +- .../applets/collections/api.py | 28 +- src/openedx_content/applets/components/api.py | 66 +-- src/openedx_content/applets/containers/api.py | 67 ++- src/openedx_content/applets/publishing/api.py | 404 +++++++++++------- .../applets/publishing/contextmanagers.py | 18 +- .../applets/publishing/models/draft_log.py | 12 +- src/openedx_content/applets/sections/api.py | 13 +- .../applets/subsections/api.py | 13 +- src/openedx_content/applets/units/api.py | 13 +- .../commands/add_assets_to_component.py | 14 +- .../applets/publishing/test_api.py | 240 +++++------ .../applets/publishing/test_signals.py | 4 +- 13 files changed, 463 insertions(+), 437 deletions(-) diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 3261836c3..4b4d66525 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -774,7 +774,7 @@ def _save( learning_package_obj = publishing_api.create_learning_package(**learning_package) self.learning_package_id = learning_package_obj.id - with publishing_api.bulk_draft_changes_for(learning_package_obj.id): + with publishing_api.draft_changes_for(learning_package_obj.id, self.user): self._save_components(learning_package_obj, components, component_static_files) self._save_units(learning_package_obj, containers) self._save_subsections(learning_package_obj, containers) @@ -782,7 +782,7 @@ def _save( self._save_collections(learning_package_obj, collections) publishing_api.publish_all_drafts(learning_package_obj.id) - with publishing_api.bulk_draft_changes_for(learning_package_obj.id): + with publishing_api.draft_changes_for(learning_package_obj.id, self.user): self._save_draft_versions(components, containers, component_static_files) return learning_package_obj @@ -849,7 +849,6 @@ def _save_container( # entity-container. BUT, this assumpion may not hold true v2+. container_code=entity_ref, **data, # should this be allowed to override any of the following fields? - created_by=self.user_id, container_cls=container_cls, ) container_map[entity_ref] = container # e.g. `self.units_map_by_ref[entity_ref] = unit` @@ -864,7 +863,6 @@ def _save_container( force_version_num=version_num, **valid_published, # should this be allowed to override any of the following fields? entities=children, - created_by=self.user_id, ) def _save_units(self, learning_package, containers): @@ -915,7 +913,6 @@ def _save_draft_versions(self, components, containers, component_static_files): # Drafts can diverge from published, so we allow ignoring previous media # Use case: published v1 had files A, B; draft v2 only has file A ignore_previous_media=True, - created_by=self.user_id, **valid_draft ) @@ -936,7 +933,6 @@ def _process_draft_containers( **valid_draft, # should this be allowed to override any of the following fields? entities=children, force_version_num=version_num, - created_by=self.user_id, ) _process_draft_containers(Unit, self.units_map_by_ref, children_map=self.components_map_by_ref) diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index b0b4a43f1..5bfcbc4bb 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -74,25 +74,29 @@ def create_collection( collection_code: str, *, title: str, - created_by: int | None, + created: publishing_api.DatetimeOrAuto = publishing_api.DATETIME_AUTO, + created_by: publishing_api.AuthorOrAuto = publishing_api.AUTHOR_AUTO, description: str = "", enabled: bool = True, ) -> Collection: """ Create a new Collection """ + created_dt = publishing_api.resolve_datetime(learning_package_id, created) + created_by_id = publishing_api.resolve_author(learning_package_id, created_by) collection = Collection( learning_package_id=learning_package_id, collection_code=collection_code, title=title, - created_by_id=created_by, + created=created_dt, + created_by_id=created_by_id, description=description, enabled=enabled, ) collection.full_clean() collection.save() if enabled: - _queue_change_event(collection, created=True, user_id=created_by) + _queue_change_event(collection, created=True, user_id=created_by_id) return collection @@ -281,7 +285,9 @@ def get_collections(learning_package_id: LearningPackage.ID, enabled: bool | Non def set_collections( publishable_entity: PublishableEntity, collection_qset: QuerySet[Collection], - created_by: int | None = None, + *, + created: publishing_api.DatetimeOrAuto = publishing_api.DATETIME_AUTO, + created_by: publishing_api.AuthorOrAuto = publishing_api.AUTHOR_AUTO, ) -> set[Collection]: """ Set collections for a given publishable entity. @@ -304,10 +310,18 @@ def set_collections( # Clear other collections for given entity and add only new collections from collection_qset removed_collections = set(r.collection for r in current_relations.exclude(collection__in=collection_qset)) new_collections = set(collection_qset.exclude(id__in=current_relations.values_list("collection", flat=True))) + + created_dt = publishing_api.resolve_datetime( + publishable_entity.learning_package_id, created + ) + created_by_id = publishing_api.resolve_author( + publishable_entity.learning_package_id, created_by + ) + # Triggers a m2m_changed signal publishable_entity.collections.set( objs=collection_qset, - through_defaults={"created_by_id": created_by}, + through_defaults={"created_by_id": created_by_id, "created": created_dt}, ) # Update modified date: affected_collections = removed_collections | new_collections @@ -324,8 +338,8 @@ def set_collections( ): # TODO: test performance of this and potentially send these async if > 1 affected collection. if collection.id in removed_ids: - _queue_change_event(collection, entities_removed=[publishable_entity.id], user_id=created_by) + _queue_change_event(collection, entities_removed=[publishable_entity.id], user_id=created_by_id) else: - _queue_change_event(collection, entities_added=[publishable_entity.id], user_id=created_by) + _queue_change_event(collection, entities_added=[publishable_entity.id], user_id=created_by_id) return affected_collections diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index fc4b1e843..a925a7376 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -79,10 +79,10 @@ def create_component( /, component_type: ComponentType, component_code: str, - created: datetime, - created_by: int | None, *, can_stand_alone: bool = True, + created: publishing_api.DatetimeOrAuto = publishing_api.DATETIME_AUTO, + created_by: publishing_api.AuthorOrAuto = publishing_api.AUTHOR_AUTO, ) -> Component: """ Create a new Component (an entity like a Problem or Video). @@ -114,8 +114,6 @@ def create_component_version( /, version_num: int, title: str, - created: datetime, - created_by: int | None, *, media: dict[str, Media.ID | Media | bytes] | None = None, ) -> ComponentVersion: @@ -132,21 +130,27 @@ def create_component_version( almost always want to create a Media object first in actual app code, because that will give you better control over the MIME type and storage specifics (file vs. database). + + Must be called inside `with draft_changes_for(...):` """ - with atomic(): - publishable_entity_version = publishing_api.create_publishable_entity_version( - component_id, - version_num=version_num, - title=title, - created=created, - created_by=created_by, - ) - component_version = ComponentVersion.objects.create( - publishable_entity_version=publishable_entity_version, - component_id=component_id, + publishable_entity_version = publishing_api.create_publishable_entity_version( + component_id, + version_num=version_num, + title=title, + ) + component_version = ComponentVersion.objects.create( + publishable_entity_version=publishable_entity_version, + component_id=component_id, + ) + if media: + _set_component_version_media( + component_version, + media, + created=publishing_api.resolve_datetime( + publishable_entity_version.entity.learning_package_id, + publishing_api.DATETIME_AUTO, + ), ) - if media: - _set_component_version_media(component_version, media, created=created) return component_version @@ -155,9 +159,7 @@ def create_next_component_version( component_id: Component.ID, /, media_to_replace: dict[str, Media.ID | Media | bytes | None], - created: datetime, title: str | None = None, - created_by: int | None = None, *, force_version_num: int | None = None, ignore_previous_media: bool = False, @@ -169,9 +171,7 @@ def create_next_component_version( component_id (int): The primary key of the Component to version. media_to_replace (dict): Mapping of file keys to Media IDs, None (for deletion), or bytes (for new file media). - created (datetime): The creation timestamp for the new version. title (str, optional): Title for the new version. If None, uses the previous version's title. - created_by (int, optional): User ID of the creator. force_version_num (int, optional): If provided, overrides the automatic version number increment and sets this version's number explicitly. Use this if you need to restore or import a version with a specific version number, such as during data migration or when synchronizing with external systems. @@ -195,8 +195,7 @@ def create_next_component_version( Using `None` for a value in this dict means to delete that key in the next version. - Make sure to wrap the function call on a atomic statement: - ``with transaction.atomic():`` + Must be called inside `with draft_changes_for(...):` It is okay to mark entries for deletion that don't exist. For instance, if a version has ``a.txt`` and ``b.txt``, sending a ``media_to_replace`` value @@ -239,8 +238,6 @@ def create_next_component_version( component_id, version_num=next_version_num, title=title, - created=created, - created_by=created_by, ) component_version = ComponentVersion.objects.create( publishable_entity_version=publishable_entity_version, @@ -268,41 +265,44 @@ def create_next_component_version( if media is not None # "media is None" means "delete this" } - _set_component_version_media(component_version, paths_to_media, created) + _set_component_version_media( + component_version, + paths_to_media, + created=publishing_api.resolve_datetime( + publishable_entity_version.entity.learning_package_id, + publishing_api.DATETIME_AUTO, + ), + ) return component_version -def create_component_and_version( # pylint: disable=too-many-positional-arguments +def create_component_and_version( learning_package_id: LearningPackage.ID, /, component_type: ComponentType, component_code: str, title: str, - created: datetime, - created_by: int | None = None, *, can_stand_alone: bool = True, media: dict[str, Media.ID | Media | bytes] | None = None, ) -> tuple[Component, ComponentVersion]: """ Create a Component and associated ComponentVersion atomically. + + Must be called inside `with draft_changes_for(...):` """ with atomic(): component = create_component( learning_package_id, component_type, component_code, - created, - created_by, can_stand_alone=can_stand_alone, ) component_version = create_component_version( component.id, version_num=1, title=title, - created=created, - created_by=created_by, media=media or {}, ) diff --git a/src/openedx_content/applets/containers/api.py b/src/openedx_content/applets/containers/api.py index 8398abc47..31e61a84b 100644 --- a/src/openedx_content/applets/containers/api.py +++ b/src/openedx_content/applets/containers/api.py @@ -5,7 +5,6 @@ from __future__ import annotations from dataclasses import dataclass -from datetime import datetime from enum import Enum from typing import Iterable @@ -135,11 +134,11 @@ def parse(entities: EntityListInput) -> list[ParsedEntityReference]: def create_container( learning_package_id: LearningPackage.ID, container_code: str, - created: datetime, - created_by: int | None, *, container_cls: type[ContainerModel], can_stand_alone: bool = True, + created: publishing_api.DatetimeOrAuto = publishing_api.DATETIME_AUTO, + created_by: publishing_api.AuthorOrAuto = publishing_api.AUTHOR_AUTO, ) -> ContainerModel: """ Create a new container. @@ -148,8 +147,8 @@ def create_container( learning_package_id: The ID of the learning package that contains the container. container_code: A local slug identifier for the container, unique within the learning package (regardless of container type). - created: The date and time the container was created. - created_by: The ID of the user who created the container + created: The date and time the container was created, or DATETIME_AUTO + created_by: The ID of the user who created the container, or AUTHOR_AUTO container_cls: The subclass of container to create (e.g. `Unit`) can_stand_alone: Set to False when created as part of containers @@ -245,11 +244,9 @@ def _create_container_version( *, title: str, entity_list: EntityList, - created: datetime, - created_by: int | None, ) -> ContainerVersion: """ - Private internal method for logic shared by create_container_version() and + Private internal method for logic shared juby create_container_version() and create_next_container_version(). """ # validate entity_list using the type implementation: @@ -277,8 +274,6 @@ def _create_container_version( container.publishable_entity_id, version_num=version_num, title=title, - created=created, - created_by=created_by, dependencies=[entity_row.entity_id for entity_row in entity_list.rows if entity_row.is_unpinned()], ) container_version = version_type.objects.create( @@ -297,8 +292,6 @@ def create_container_version( *, title: str, entities: EntityListInput, - created: datetime, - created_by: int | None, ) -> ContainerVersion: """ Create a new container version. @@ -312,11 +305,11 @@ def create_container_version( `PublishableEntityVersionMixin` to pin to a specific version. Pass `PublishableEntity` or objects that use `PublishableEntityMixin` for unpinned. - 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. + + Must be called inside `with draft_changes_for(...):` """ assert title is not None assert entities is not None @@ -331,8 +324,6 @@ def create_container_version( version_num, title=title, entity_list=entity_list, - created=created, - created_by=created_by, ) return container_version @@ -345,8 +336,6 @@ def create_container_and_version( title: str, container_cls: type[ContainerModel], entities: EntityListInput | None = None, - created: datetime, - created_by: int | None = None, can_stand_alone: bool = True, ) -> tuple[ContainerModel, ContainerVersionModel]: """ @@ -362,27 +351,22 @@ def create_container_and_version( `PublishableEntityVersionMixin` to pin to a specific version. Pass `PublishableEntity` or objects that use `PublishableEntityMixin` for unpinned. Pass `None` for "no change". - created: The creation date. - created_by: The ID of the user who created the container. can_stand_alone: Set to False when created as part of containers + + Must be called inside `with draft_changes_for(...):` """ - with atomic(savepoint=False): - container = create_container( - learning_package_id, - container_code, - created, - created_by, - can_stand_alone=can_stand_alone, - container_cls=container_cls, - ) - container_version: ContainerVersionModel = create_container_version( # type: ignore[assignment] - container.id, - 1, - title=title, - entities=entities or [], - created=created, - created_by=created_by, - ) + container = create_container( + learning_package_id, + container_code, + can_stand_alone=can_stand_alone, + container_cls=container_cls, + ) + container_version: ContainerVersionModel = create_container_version( # type: ignore[assignment] + container.id, + 1, + title=title, + entities=entities or [], + ) return container, container_version @@ -415,6 +399,7 @@ def create_next_entity_list( Returns: The newly created entity list. + Must be called inside `with draft_changes_for(...):` """ parsed_entities = ParsedEntityReference.parse(entities) # Do a quick check that the given entities are in the right learning package: @@ -458,8 +443,6 @@ def create_next_container_version( *, title: str | None = None, entities: EntityListInput | None = None, - created: datetime, - created_by: int | None, entities_action: ChildrenEntitiesAction = ChildrenEntitiesAction.REPLACE, force_version_num: int | None = None, ) -> ContainerVersion: @@ -481,8 +464,6 @@ def create_next_container_version( `PublishableEntityVersionMixin` to pin to a specific version. Pass `PublishableEntity` or objects that use `PublishableEntityMixin` for unpinned. Pass `None` for "no change". - created: The date and time the container version was created. - created_by: The ID of the user who created the container version. force_version_num (int, optional): If provided, overrides the automatic version number increment and sets this version's number explicitly. Use this if you need to restore or import a version with a specific version number, such as during data migration or when synchronizing with external systems. @@ -495,6 +476,8 @@ def create_next_container_version( If you need to set a specific version number (for example, when restoring from backup, importing legacy data, or synchronizing with another system), use force_version_num to override the default behavior. + + Must be called inside `with draft_changes_for(...):` """ with atomic(): if isinstance(container, int): @@ -523,8 +506,6 @@ def create_next_container_version( next_version_num, title=title if title is not None else last_version.title, entity_list=next_entity_list, - created=created, - created_by=created_by, ) # reset any potentially cached 'container.versioning.draft' value on the passed 'container' instance, since we've diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 07d0696b4..2c3bb16e3 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -7,12 +7,12 @@ from __future__ import annotations -from contextlib import nullcontext from datetime import datetime, timezone from functools import partial -from typing import ContextManager, Optional, cast +from typing import Literal, Optional, TypeAlias, cast from django.contrib.auth import get_user_model +from django.contrib.auth.models import AbstractUser, AnonymousUser from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, OuterRef, Prefetch, Q, QuerySet, Subquery from django.db.transaction import atomic, on_commit @@ -45,6 +45,11 @@ # start with an underscore AND it is not in __all__, that function is considered # to be callable only by other applets in the openedx_content package. __all__ = [ + "DATETIME_AUTO", + "DatetimeOrAuto", + "UserID", + "AUTHOR_AUTO", + "AuthorOrAuto", "get_learning_package", "get_learning_package_by_ref", "create_learning_package", @@ -72,10 +77,125 @@ "reset_drafts_to_published", "register_publishable_models", "filter_publishable_entities", - "bulk_draft_changes_for", + "draft_changes_for", ] +DATETIME_AUTO: Literal["DATETIME_AUTO"] = "DATETIME_AUTO" +""" +Automatically choose a timestamp for a creation, deletion, edit, or change. + +Within a `draft_changes_for` context, `DATETIME_AUTO` indicates to use the +context's timestamp. Outside of `draft_changes_for`, `DATETIME_AUTO` indicates +to the current time (`datetime.now`). + +`DATETIME_AUTO` is a reasonable default for most situations. +""" + + +type DatetimeOrAuto = datetime | Literal["DATETIME_AUTO"] +""" +How to choose a datetime indicating when an operation occured. + +This is either a specific `datetime`, or it is `DATETIME_AUTO`. +When in doubt, `DATETIME_AUTO` is a reasonable default. +""" + + +UserID: TypeAlias = int +""" +UserID is an integer. + +A UserID is meant to hold a primary key of a User object, but this is not +enforced by the type checker. Future versions of openedx-core may enforce +this more strictly by making UserID a NewType (subclass) of int. +""" + + +AUTHOR_AUTO: Literal["AUTHOR_AUTO"] = "AUTHOR_AUTO" +""" +Attribute the operation to the same user as the enclosing `draft_changes_for` context. + +Outside of a context, using `AUTHOR_AUTO` is meaningless and will result in a `ValueError`. +""" + + +type AuthorOrAuto = ( + # Attribute to a specific user: + UserID | AbstractUser + # Attribue to nobody: + | None | AnonymousUser + # Attribute to the same user as the enclosing draft_changes_for: + | Literal["AUTHOR_AUTO"] +) +""" +How to attribute an operation (creation, edit, deletion, change) to a user. + +For attributable changes, this could be a User, or its database ID. + +For changes without any attributable author (e.g. backfills), this could be None +or an AnonymousUser, both of which map to NULL in the database. This should be +reserved for special cases--most changes have a concrete author! + +Finally, this could be AUTHOR_AUTO, which means "use the same author (or lack +thereof) that the draft change context is using." Most functions default to AUTHOR_AUTO. +""" + + +def resolve_datetime( + learning_package_id: LearningPackage.ID, + dt: DatetimeOrAuto, +) -> datetime: + """ + Convert a Timestamp specifier into a concerete datetime to be saved to the DB. + + If `timestamp is DATETIME_AUTO`, then return either the datetime of the + enclosing `draft_changes_for` context, or `datetime.now` if there is + no enclosing context. + + This function can be used by other openedx_content applets, but it is not part of the public API. + """ + if dt is DATETIME_AUTO: + if ctx := DraftChangeLogContext.get_active_draft_change_log(learning_package_id): + return ctx.changed_at + return datetime.now(tz=timezone.utc) + assert isinstance(dt, datetime) + return dt + + +def resolve_author( + learning_package_id: LearningPackage.ID, + author: AuthorOrAuto, +) -> UserID | None: + """ + Convert a Author specifier into a concerete user ID (or None) to be saved to the DB. + + If `author is AUTHOR_AUTO`, then return either the author of the enclosing + `draft_changes_for` context. Raises `ValueError` if there is no + enclosing context. + + This function can be used by other openedx_content applets, but it is not part of the public API. + """ + if author is AUTHOR_AUTO: + if ctx := DraftChangeLogContext.get_active_draft_change_log(learning_package_id): + return ctx.changed_by.id if ctx.changed_by else None + else: + raise ValueError( + "An author (other than AUTHOR_AUTO) must be specified when changing content " + "outside of a `draft_changes_for` context." + ) + elif isinstance(author, AnonymousUser): + return None + elif isinstance(author, AbstractUser): + assert isinstance(author.pk, int) + return author.pk + elif author: + assert isinstance(author, int) + return author + else: + return None + + def get_learning_package(learning_package_id: LearningPackage.ID, /) -> LearningPackage: """ Get LearningPackage by ID. @@ -93,7 +213,7 @@ def get_learning_package_by_ref(package_ref: str) -> LearningPackage: def create_learning_package( - package_ref: str, title: str, description: str = "", created: datetime | None = None + package_ref: str, title: str, description: str = "", created: DatetimeOrAuto = DATETIME_AUTO, ) -> LearningPackage: """ Create a new LearningPackage. @@ -104,7 +224,7 @@ def create_learning_package( * django.core.exceptions.ValidationError """ - if not created: + if created is DATETIME_AUTO: created = datetime.now(tz=timezone.utc) package = LearningPackage( @@ -134,7 +254,7 @@ def update_learning_package( package_ref: str | None = None, title: str | None = None, description: str | None = None, - updated: datetime | None = None, + updated: DatetimeOrAuto = DATETIME_AUTO, ) -> LearningPackage: """ Make an update to LearningPackage metadata. @@ -155,11 +275,7 @@ def update_learning_package( if description is not None: lp.description = description - # updated is a bit different–we auto-generate it if it's not explicitly - # passed in. - if updated is None: - updated = datetime.now(tz=timezone.utc) - lp.updated = updated + lp.updated = resolve_datetime(learning_package_id, updated) lp.save() @@ -191,9 +307,8 @@ def create_publishable_entity( learning_package_id: LearningPackage.ID, /, entity_ref: str, - created: datetime, - # User ID who created this - created_by: int | None, + created: DatetimeOrAuto = DATETIME_AUTO, + created_by: AuthorOrAuto = AUTHOR_AUTO, *, can_stand_alone: bool = True, ) -> PublishableEntity: @@ -202,12 +317,15 @@ def create_publishable_entity( You'd typically want to call this right before creating your own content model that points to it. + + Must be called inside `with draft_changes_for(...):` """ + return PublishableEntity.objects.create( learning_package_id=learning_package_id, entity_ref=entity_ref, - created=created, - created_by_id=created_by, + created=resolve_datetime(learning_package_id, created), + created_by_id=resolve_author(learning_package_id, created_by), can_stand_alone=can_stand_alone, ) @@ -217,8 +335,6 @@ def create_publishable_entity_version( /, version_num: int, title: str, - created: datetime, - created_by: int | None, *, dependencies: list[PublishableEntity.ID] | None = None, ) -> PublishableEntityVersion: @@ -228,13 +344,14 @@ def create_publishable_entity_version( You'd typically want to call this right before creating your own content version model that points to it. """ + learning_package_id = get_publishable_entity(entity_id).learning_package_id with atomic(savepoint=False): version = PublishableEntityVersion.objects.create( entity_id=entity_id, version_num=version_num, title=title, - created=created, - created_by_id=created_by, + created=resolve_datetime(learning_package_id, DATETIME_AUTO), + created_by_id=resolve_author(learning_package_id, AUTHOR_AUTO), ) if dependencies: # Validate that dependencies are from the same learning package: @@ -247,12 +364,7 @@ def create_publishable_entity_version( # Store dependencies: set_version_dependencies(version.id, dependencies) - set_draft_version( - entity_id, - version.id, - set_at=created, - set_by=created_by, - ) + set_draft_version(entity_id, version.id) return version @@ -409,8 +521,8 @@ def publish_all_drafts( learning_package_id: LearningPackage.ID, /, message="", - published_at: datetime | None = None, - published_by: int | None = None + published_at: DatetimeOrAuto = DATETIME_AUTO, + published_by: AuthorOrAuto = AUTHOR_AUTO, ) -> PublishLog: """ Publish everything that is a Draft and is not already published. @@ -463,8 +575,8 @@ def publish_from_drafts( /, draft_qset: QuerySet[Draft], message: str = "", - published_at: datetime | None = None, - published_by: int | None = None, # User.id + published_at: DatetimeOrAuto = DATETIME_AUTO, + published_by: AuthorOrAuto = AUTHOR_AUTO, *, publish_dependencies: bool = True, ) -> PublishLog: @@ -474,9 +586,10 @@ def publish_from_drafts( By default, this will also publish all dependencies (e.g. unpinned children) of the Drafts that are passed in. """ - if published_at is None: - published_at = datetime.now(tz=timezone.utc) - + if DraftChangeLogContext.get_active_draft_change_log(learning_package_id) is not None: + raise ValidationError("Cannot publish while in draft_changes_for().") + published_at = resolve_datetime(learning_package_id, published_at) + published_by = resolve_author(learning_package_id, published_by) with atomic(): if publish_dependencies: dependency_drafts_qsets = _get_dependencies_with_unpublished_changes(draft_qset) @@ -885,8 +998,6 @@ def set_draft_version( draft_or_id: Draft | PublishableEntity.ID, publishable_entity_version_pk: int | None, /, - set_at: datetime | None = None, - set_by: int | None = None, # User.id ) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. @@ -906,14 +1017,13 @@ def set_draft_version( This function will create DraftSideEffect entries and properly add any containers that may have been affected by this draft update, UNLESS it is - called from within a bulk_draft_changes_for block. If it is called from - inside a bulk_draft_changes_for block, it will not add side-effects for - containers, as bulk_draft_changes_for will automatically do that when the - block exits. - """ - if set_at is None: - set_at = datetime.now(tz=timezone.utc) + called from within a draft_changes_for block. If it is called from + inside a draft_changes_for block, it will not add side-effects for + containers, as draft_changes_for will automatically do that when the + block exits. @@TODO update this docstring + Must be called inside `with draft_changes_for(...):` + """ with atomic(savepoint=False): if isinstance(draft_or_id, Draft): draft = draft_or_id @@ -946,77 +1056,56 @@ def set_draft_version( f"the PublishableEntity ({repr(draft.entity)})." ) - # Check to see if we're inside a context manager for an active - # DraftChangeLog (i.e. what happens if the caller is using the public - # bulk_draft_changes_for() API call), or if we have to make our own. + # Ensure we're inside a context manager for an active DraftChangeLog + # (i.e. what happens if the caller is using the public + # draft_changes_for() API call). learning_package_id = draft.entity.learning_package_id - active_change_log = DraftChangeLogContext.get_active_draft_change_log( + active_change_log = DraftChangeLogContext.get_active_draft_change_log_or_raise( learning_package_id ) - if active_change_log: - draft_log_record = _add_to_existing_draft_change_log( - active_change_log, - draft.entity_id, - old_version_id=old_version_id, - new_version_id=publishable_entity_version_pk, - ) - if draft_log_record: - # Normal case: a DraftChangeLogRecord was created or updated. - draft.draft_log_record = draft_log_record - else: - # Edge case: this change cancelled out other changes, and the - # net effect is that the DraftChangeLogRecord shouldn't exist, - # i.e. the version at the start and end of the DraftChangeLog is - # the same. In that case, _add_to_existing_draft_change_log will - # delete the log record for this Draft state, and we have to - # look for the most recently created DraftLogRecord from another - # DraftChangeLog. This value may be None. - # - # NOTE: There may be some weird edge cases here around soft- - # deletions and modifications of the same Draft entry in nested - # bulk_draft_changes_for() calls. I haven't thought that through - # yet--it might be better to just throw an error rather than try - # to accommodate it. - draft.draft_log_record = ( - DraftChangeLogRecord.objects - .filter(entity_id=draft.entity_id) - .order_by("-pk") - .first() - ) - draft.save() - - # We also *don't* create container side effects here because there - # may be many changes in this DraftChangeLog, some of which haven't - # been made yet. It wouldn't make sense to create a side effect that - # says, "this Unit changed because this Component in it changed" if - # we were changing that same Unit later on in the same - # DraftChangeLog, because that new Unit version might not even - # include that child Component. Also, calculating side-effects is - # expensive, and would result in a lot of wasted queries if we did - # it for every change will inside an active change log context. - # - # Therefore we'll let DraftChangeLogContext do that work when it - # exits its context. + draft_log_record = _add_to_existing_draft_change_log( + active_change_log, + draft.entity_id, + old_version_id=old_version_id, + new_version_id=publishable_entity_version_pk, + ) + if draft_log_record: + # Normal case: a DraftChangeLogRecord was created or updated. + draft.draft_log_record = draft_log_record else: - # This means there is no active DraftChangeLog, so we create our own - # and add our DraftChangeLogRecord to it. This has the very minor - # optimization that we don't have to check for an existing - # DraftChangeLogRecord, because we know it can't exist yet. - change_log = DraftChangeLog.objects.create( - learning_package_id=learning_package_id, - changed_at=set_at, - changed_by_id=set_by, - ) - draft.draft_log_record = DraftChangeLogRecord.objects.create( - draft_change_log=change_log, - entity_id=draft.entity_id, - old_version_id=old_version_id, - new_version_id=publishable_entity_version_pk, + # Edge case: this change cancelled out other changes, and the + # net effect is that the DraftChangeLogRecord shouldn't exist, + # i.e. the version at the start and end of the DraftChangeLog is + # the same. In that case, _add_to_existing_draft_change_log will + # delete the log record for this Draft state, and we have to + # look for the most recently created DraftLogRecord from another + # DraftChangeLog. This value may be None. + # + # NOTE: There may be some weird edge cases here around soft- + # deletions and modifications of the same Draft entry in nested + # draft_changes_for() calls. I haven't thought that through + # yet--it might be better to just throw an error rather than try + # to accommodate it. + draft.draft_log_record = ( + DraftChangeLogRecord.objects + .filter(entity_id=draft.entity_id) + .order_by("-pk") + .first() ) - draft.save() - _create_side_effects_for_change_log(change_log) - # Send out an event immediately after this database transaction commits, since this is an isolated change. - _emit_event_for_change_log(change_log, timestamp=set_at, user_id=set_by) + draft.save() + + # We also *don't* create container side effects here because there + # may be many changes in this DraftChangeLog, some of which haven't + # been made yet. It wouldn't make sense to create a side effect that + # says, "this Unit changed because this Component in it changed" if + # we were changing that same Unit later on in the same + # DraftChangeLog, because that new Unit version might not even + # include that child Component. Also, calculating side-effects is + # expensive, and would result in a lot of wasted queries if we did + # it for every change will inside an active change log context. + # + # Therefore we'll let DraftChangeLogContext do that work when it + # exits its context. def _add_to_existing_draft_change_log( @@ -1060,7 +1149,7 @@ def _add_to_existing_draft_change_log( # Special case: This change undoes the previous change(s). The value # in change.old_version_id represents the Draft version before the # DraftChangeLog was started, regardless of how many times we've - # changed it since we entered the bulk_draft_changes_for() context. + # changed it since we entered the draft_changes_for() context. # If we get here in the code, it means that we're now setting the # Draft version of this entity to be exactly what it was at the # start, and we should remove it entirely from the DraftChangeLog. @@ -1075,7 +1164,7 @@ def _add_to_existing_draft_change_log( else: # Normal case: We update the new_version, but leave the old_version # as is. The old_version represents what the Draft was pointing to - # when the bulk_draft_changes_for() context started, so it persists + # when the draft_changes_for() context started, so it persists # if we change the same entity multiple times in the DraftChangeLog. change.new_version_id = new_version_id change.save() @@ -1550,7 +1639,10 @@ def hash_for_log_record( return digest -def soft_delete_draft(publishable_entity_id: PublishableEntity.ID, /, deleted_by: int | None = None) -> None: +def soft_delete_draft( + publishable_entity_id: PublishableEntity.ID, + /, +) -> None: """ Sets the Draft version to None. @@ -1559,15 +1651,15 @@ def soft_delete_draft(publishable_entity_id: PublishableEntity.ID, /, deleted_by version. No version data is actually deleted, so restoring is just a matter of pointing the Draft back to the most recent ``PublishableEntityVersion`` for a given ``PublishableEntity``. + + Must be called inside `with draft_changes_for(...):` """ - return set_draft_version(publishable_entity_id, None, set_by=deleted_by) + return set_draft_version(publishable_entity_id, None) def reset_drafts_to_published( learning_package_id: LearningPackage.ID, /, - reset_at: datetime | None = None, - reset_by: int | None = None, # User.id ) -> None: """ Reset all Drafts to point to the most recently Published versions. @@ -1592,9 +1684,12 @@ def reset_drafts_to_published( it's important that the code creating the "next" version_num looks at the latest version created for a PublishableEntity (its ``latest`` attribute), rather than basing it off of the version that Draft points to. + + Must be called inside `with draft_changes_for(...):` """ - if reset_at is None: - reset_at = datetime.now(tz=timezone.utc) + # Ensure we're inside `with draft_changes_for(...)` (and, thus, inside a + # transaction). + DraftChangeLogContext.get_active_draft_change_log_or_raise(learning_package_id) # These are all the drafts that are different from the published versions. draft_qset = Draft.objects \ @@ -1616,31 +1711,18 @@ def reset_drafts_to_published( if not draft_qset: return - active_change_log = DraftChangeLogContext.get_active_draft_change_log(learning_package_id) - - # If there's an active DraftChangeLog, we're already in a transaction, so - # there's no need to open a new one. - tx_context: ContextManager - if active_change_log: - tx_context = nullcontext() - else: - tx_context = bulk_draft_changes_for( - learning_package_id, changed_at=reset_at, changed_by=reset_by - ) - - with tx_context: - # Note: We can't do an .update with a F() on a joined field in the ORM, - # so we have to loop through the drafts individually to reset them - # anyhow. We can rework this into a bulk update or custom SQL if it - # becomes a performance issue, as long as we also port over the - # bookkeeping code in set_draft_version. - for draft in draft_qset: - if hasattr(draft.entity, 'published'): - published_version_id = draft.entity.published.version_id - else: - published_version_id = None + # Note: We can't do an .update with a F() on a joined field in the ORM, + # so we have to loop through the drafts individually to reset them + # anyhow. We can rework this into a bulk update or custom SQL if it + # becomes a performance issue, as long as we also port over the + # bookkeeping code in set_draft_version. + for draft in draft_qset: + if hasattr(draft.entity, 'published'): + published_version_id = draft.entity.published.version_id + else: + published_version_id = None - set_draft_version(draft, published_version_id) + set_draft_version(draft, published_version_id) def register_publishable_models( @@ -1713,10 +1795,10 @@ def get_published_version_as_of( return record.new_version if record else None -def bulk_draft_changes_for( +def draft_changes_for( learning_package_id: LearningPackage.ID, - changed_by: int | None = None, - changed_at: datetime | None = None + changed_by: UserID | AbstractUser | AnonymousUser | None, + changed_at: DatetimeOrAuto = DATETIME_AUTO, ) -> DraftChangeLogContext: """ Context manager to do a single batch of Draft changes in. @@ -1732,29 +1814,31 @@ def bulk_draft_changes_for( Example:: - with bulk_draft_changes_for(learning_package.id): + with draft_changes_for(learning_package.id, changed_by=request.user.id): for section in course: update_section_drafts(learning_package.id, section) - - If you make a change to an entity *without* using this context manager, then - the individual change (and its side effects) will be automatically wrapped - in a one-off change context. For example, this:: - - update_one_component(component.learning_package, component) - - is identical to this:: - - with bulk_draft_changes_for(component.learning_package.id): - update_one_component(component.learning_package.id, component) """ - if not changed_at: - changed_at = datetime.now(tz=timezone.utc) + changed_at_dt: datetime + if changed_at is DATETIME_AUTO: + changed_at_dt = datetime.now(tz=timezone.utc) + else: + assert isinstance(changed_at, datetime) + changed_at_dt = changed_at + changed_by_id: UserID | None + if isinstance(changed_by, AnonymousUser): + changed_by_id = None + elif isinstance(changed_by, AbstractUser): + assert isinstance(changed_by.pk, int) + changed_by_id = changed_by.pk + elif changed_by: + assert isinstance(changed_by, int) + changed_by_id = changed_by return DraftChangeLogContext( learning_package_id, - changed_at=changed_at, - changed_by=changed_by, + changed_at=changed_at_dt, + changed_by=changed_by_id, exit_callbacks=[ _create_side_effects_for_change_log, - partial(_emit_event_for_change_log, timestamp=changed_at, user_id=changed_by), + partial(_emit_event_for_change_log, timestamp=changed_at_dt, user_id=changed_by_id), ] ) diff --git a/src/openedx_content/applets/publishing/contextmanagers.py b/src/openedx_content/applets/publishing/contextmanagers.py index 0dcdc1dbb..cf95482ee 100644 --- a/src/openedx_content/applets/publishing/contextmanagers.py +++ b/src/openedx_content/applets/publishing/contextmanagers.py @@ -2,7 +2,7 @@ Context Managers for internal use in the publishing app. Do not use this directly outside the publishing app. Use the public API's -bulk_draft_changes_for instead (which will invoke this internally). +draft_changes_for instead (which will invoke this internally). """ from __future__ import annotations @@ -20,7 +20,7 @@ class DraftChangeLogContext(Atomic): Context manager for batching draft changes into DraftChangeChangeLogs. 🛑 This is a PRIVATE implementation. Outside of the publishing app, clients - should use the bulk_draft_changes_for() API call instead of using this + should use the draft_changes_for() API call instead of using this manager directly, since this is a bit experimental and the implementation may shift around a bit. @@ -30,7 +30,7 @@ class DraftChangeLogContext(Atomic): layers higher in the stack than doing the draft changes. For instance, imagine:: - with bulk_draft_changes_for(learning_package.id): + with draft_changes_for(learning_package.id, request.user.id): for section in course: update_section_drafts(learning_package_id, section) @@ -74,6 +74,18 @@ def __init__( # part of the code that doesn't understand what containers are. self.exit_callbacks = exit_callbacks or [] + @classmethod + def get_active_draft_change_log_or_raise(cls, learning_package_id: int) -> DraftChangeLog: + """ + Get the DraftChangeLogContext, or raise ValueError if there is none. + + Appropriate to call for operations which must be associated with a DraftChangeLog, + such as create_publishable_entity_version. + """ + if ctx := cls.get_active_draft_change_log(learning_package_id): + return ctx + raise ValueError("Draft changes can only be made inside a `with draft_changes_for(...):` block.") + @classmethod def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog | None: """ diff --git a/src/openedx_content/applets/publishing/models/draft_log.py b/src/openedx_content/applets/publishing/models/draft_log.py index 18ebf313b..e7b83e93b 100644 --- a/src/openedx_content/applets/publishing/models/draft_log.py +++ b/src/openedx_content/applets/publishing/models/draft_log.py @@ -62,7 +62,7 @@ class Draft(models.Model): # addition to this data model, so we have to allow null values for the # initial migration. But making this nullable also has another advantage, # in that it allows us to set the draft_log_record to the most recent change - # while inside a bulk_draft_changes_for operation, and then delete that log + # while inside a draft_changes_for operation, and then delete that log # record if it is undone in the same bulk operation. draft_log_record = models.ForeignKey( "DraftChangeLogRecord", @@ -206,7 +206,7 @@ class DraftChangeLogRecord(models.Model): - (v1, v4): Normal edit in draft This could also technically happen if we change the same entity more than - once in the the same bulk_draft_changes_for() context, thereby putting + once in the the same draft_changes_for() context, thereby putting them into the same DraftChangeLog, which forces us to squash the changes together into one DraftChangeLogRecord. @@ -330,13 +330,13 @@ class DraftSideEffect(models.Model): The child change is still affecting the parent container, whether the container happens to be changing for other reasons as well. Whether a parent -child relationship exists or not depends on the draft state of the - container at the *end* of a bulk_draft_changes_for context. To give concrete + container at the *end* of a draft_changes_for context. To give concrete examples: Setup: A Unit version U1.v1 has defined C1 to be a child. The current draft version of C1 is C1.v1. - Scenario 1: In the a bulk_draft_changes_for context, we edit C1 so that the + Scenario 1: In the a draft_changes_for context, we edit C1 so that the draft version of C1 is now C1.v2. Result: - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - a DraftChangeLogRecord is created for U1.v1 -> U1.v1 @@ -346,7 +346,7 @@ class DraftSideEffect(models.Model): has *changed* in some way because of the side effect of its child being edited. - Scenario 2: In a bulk_draft_changes_for context, we edit C1 so that the + Scenario 2: In a draft_changes_for context, we edit C1 so that the draft version of C1 is now C1.v2. In the same context, we edit U1's metadata so that the draft version of U1 is now U1.v2. U1.v2 still lists C1 as a child entity. Result: @@ -355,7 +355,7 @@ class DraftSideEffect(models.Model): - a DraftSideEffect is created with cause (C1.v1 -> C1.v2) and effect (U1.v1 -> U1.v2) - Scenario 3: In a bulk_draft_changes_for context, we edit C1 so that the + Scenario 3: In a draft_changes_for context, we edit C1 so that the draft version of C1 is now C1.v2. In the same context, we edit U1's list of children so that C1 is no longer a child of U1.v2. Result: - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 diff --git a/src/openedx_content/applets/sections/api.py b/src/openedx_content/applets/sections/api.py index cdfd36e8b..32e045167 100644 --- a/src/openedx_content/applets/sections/api.py +++ b/src/openedx_content/applets/sections/api.py @@ -4,7 +4,6 @@ """ from dataclasses import dataclass -from datetime import datetime from typing import Iterable from ..containers import api as containers_api @@ -33,8 +32,6 @@ def create_section_and_version( *, title: str, subsections: Iterable[Subsection | SubsectionVersion] | None = None, - created: datetime, - created_by: int | None = None, can_stand_alone: bool = True, ) -> tuple[Section, SectionVersion]: """ @@ -43,14 +40,14 @@ def create_section_and_version( The only real purpose of this function is to rename `entities` to `subsections`, and to specify that the version returned is a `SectionVersion`. In the future, if `SectionVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ section, sv = containers_api.create_container_and_version( learning_package_id, container_code=container_code, title=title, entities=subsections, - created=created, - created_by=created_by, can_stand_alone=can_stand_alone, container_cls=Section, ) @@ -63,8 +60,6 @@ def create_next_section_version( *, title: str | None = None, subsections: Iterable[Subsection | SubsectionVersion] | None = None, - created: datetime, - created_by: int | None, ) -> SectionVersion: """ See documentation of content_api.create_next_container_version() @@ -72,6 +67,8 @@ def create_next_section_version( The only real purpose of this function is to rename `entities` to `subsections`, and to specify that the version returned is a `SectionVersion`. In the future, if `SectionVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ if isinstance(section, int): section = get_section(section) @@ -80,8 +77,6 @@ def create_next_section_version( section, title=title, entities=subsections, - created=created, - created_by=created_by, # For now, `entities_action` and `force_version_num` are unsupported but we could add them in the future. ) assert isinstance(sv, SectionVersion) diff --git a/src/openedx_content/applets/subsections/api.py b/src/openedx_content/applets/subsections/api.py index e4c24ca36..d40b1a9cb 100644 --- a/src/openedx_content/applets/subsections/api.py +++ b/src/openedx_content/applets/subsections/api.py @@ -4,7 +4,6 @@ """ from dataclasses import dataclass -from datetime import datetime from typing import Iterable from ..containers import api as containers_api @@ -33,8 +32,6 @@ def create_subsection_and_version( *, title: str, units: Iterable[Unit | UnitVersion] | None = None, - created: datetime, - created_by: int | None = None, can_stand_alone: bool = True, ) -> tuple[Subsection, SubsectionVersion]: """ @@ -43,14 +40,14 @@ def create_subsection_and_version( The only real purpose of this function is to rename `entities` to `units`, and to specify that the version returned is a `SubsectionVersion`. In the future, if `SubsectionVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ subsection, sv = containers_api.create_container_and_version( learning_package_id, container_code=container_code, title=title, entities=units, - created=created, - created_by=created_by, can_stand_alone=can_stand_alone, container_cls=Subsection, ) @@ -63,8 +60,6 @@ def create_next_subsection_version( *, title: str | None = None, units: Iterable[Unit | UnitVersion] | None = None, - created: datetime, - created_by: int | None, ) -> SubsectionVersion: """ See documentation of content_api.create_next_container_version() @@ -72,6 +67,8 @@ def create_next_subsection_version( The only real purpose of this function is to rename `entities` to `units`, and to specify that the version returned is a `SubsectionVersion`. In the future, if `SubsectionVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ if isinstance(subsection, int): subsection = get_subsection(subsection) @@ -80,8 +77,6 @@ def create_next_subsection_version( subsection, title=title, entities=units, - created=created, - created_by=created_by, # For now, `entities_action` and `force_version_num` are unsupported but we could add them in the future. ) assert isinstance(sv, SubsectionVersion) diff --git a/src/openedx_content/applets/units/api.py b/src/openedx_content/applets/units/api.py index 0fb55cfe0..d9e7e7cdd 100644 --- a/src/openedx_content/applets/units/api.py +++ b/src/openedx_content/applets/units/api.py @@ -4,7 +4,6 @@ """ from dataclasses import dataclass -from datetime import datetime from typing import Iterable from ..components.models import Component, ComponentVersion @@ -33,8 +32,6 @@ def create_unit_and_version( *, title: str, components: Iterable[Component | ComponentVersion] | None = None, - created: datetime, - created_by: int | None = None, can_stand_alone: bool = True, ) -> tuple[Unit, UnitVersion]: """ @@ -43,14 +40,14 @@ def create_unit_and_version( The only real purpose of this function is to rename `entities` to `components`, and to specify that the version returned is a `UnitVersion`. In the future, if `UnitVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ unit, uv = containers_api.create_container_and_version( learning_package_id, container_code=container_code, title=title, entities=components, - created=created, - created_by=created_by, can_stand_alone=can_stand_alone, container_cls=Unit, ) @@ -63,8 +60,6 @@ def create_next_unit_version( *, title: str | None = None, components: Iterable[Component | ComponentVersion] | None = None, - created: datetime, - created_by: int | None, ) -> UnitVersion: """ See documentation of content_api.create_next_container_version() @@ -72,6 +67,8 @@ def create_next_unit_version( The only real purpose of this function is to rename `entities` to `components`, and to specify that the version returned is a `UnitVersion`. In the future, if `UnitVersion` gets some fields that aren't on `ContainerVersion`, this function would be more important. + + Must be called inside `with draft_changes_for(...):` """ if isinstance(unit, int): unit = get_unit(unit) @@ -80,8 +77,6 @@ def create_next_unit_version( unit, title=title, entities=components, - created=created, - created_by=created_by, # For now, `entities_action` and `force_version_num` are unsupported but we could add them in the future. ) assert isinstance(uv, UnitVersion) diff --git a/src/openedx_content/management/commands/add_assets_to_component.py b/src/openedx_content/management/commands/add_assets_to_component.py index 21fc2cbe0..bf8837a47 100644 --- a/src/openedx_content/management/commands/add_assets_to_component.py +++ b/src/openedx_content/management/commands/add_assets_to_component.py @@ -5,11 +5,10 @@ asset data into the system. """ import pathlib -from datetime import datetime, timezone from django.core.management.base import BaseCommand -from ...api import create_next_component_version, get_component_by_code, get_learning_package_by_ref +from ...api import create_next_component_version, draft_changes_for, get_component_by_code, get_learning_package_by_ref class Command(BaseCommand): @@ -64,7 +63,6 @@ def handle(self, *args, **options): learning_package.id, namespace, type_name, component_code ) - created = datetime.now(tz=timezone.utc) media_path_to_content_bytes = {} for file_mapping in file_mappings: @@ -72,11 +70,11 @@ def handle(self, *args, **options): media_path_to_content_bytes[media_path] = pathlib.Path(file_path).read_bytes() if file_path else None - next_version = create_next_component_version( - component.id, - media_to_replace=media_path_to_content_bytes, - created=created, - ) + with draft_changes_for(learning_package.id, None): + next_version = create_next_component_version( + component.id, + media_to_replace=media_path_to_content_bytes, + ) self.stdout.write( f"Created v{next_version.version_num} of " diff --git a/tests/openedx_content/applets/publishing/test_api.py b/tests/openedx_content/applets/publishing/test_api.py index 157c091a7..c38fa61e8 100644 --- a/tests/openedx_content/applets/publishing/test_api.py +++ b/tests/openedx_content/applets/publishing/test_api.py @@ -310,61 +310,51 @@ def inner_reset_drafts_to_published(self, bulk: bool) -> None: wanted to make sure nothing went wrong when multiple types of reset were happening at the same time. """ - with (publishing_api.bulk_draft_changes_for(self.learning_package_1.id) if bulk else nullcontext()): + with (publishing_api.draft_changes_for(self.learning_package_1.id, None) if bulk else nullcontext()): # This is the Entity that's going to get a couple of new versions # between Draft and Published. entity_with_changes = publishing_api.create_publishable_entity( self.learning_package_1.id, "entity_with_changes", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity_with_changes.id, version_num=1, title="I'm entity_with_changes v1", - created=self.now, - created_by=None, ) # This Entity is going to remain unchanged between Draft and Published. entity_with_no_changes = publishing_api.create_publishable_entity( self.learning_package_1.id, "entity_with_no_changes", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity_with_no_changes.id, version_num=1, title="I'm entity_with_no_changes v1", - created=self.now, - created_by=None, ) # This Entity will be Published, but will then be soft-deleted. entity_with_pending_delete = publishing_api.create_publishable_entity( self.learning_package_1.id, "entity_with_pending_delete", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity_with_pending_delete.id, version_num=1, title="I'm entity_with_pending_delete v1", - created=self.now, - created_by=None, ) if bulk: - # bulk_draft_changes_for creates only one DraftChangeLog for all the preceeding operations + # draft_changes_for creates only one DraftChangeLog for all the preceeding operations assert DraftChangeLog.objects.count() == 1 # Publish! publishing_api.publish_all_drafts(self.learning_package_1.id) - with (publishing_api.bulk_draft_changes_for(self.learning_package_1.id) if bulk else nullcontext()) as changes2: + with ( + publishing_api.draft_changes_for(self.learning_package_1.id, None) if bulk else nullcontext() + ) as changes2: # Create versions 2, 3, 4 of entity_with_changes. After this, the # Published version is 1 and the Draft version is 4. for version_num in range(2, 5): @@ -372,8 +362,6 @@ def inner_reset_drafts_to_published(self, bulk: bool) -> None: entity_with_changes.id, version_num=version_num, title=f"I'm entity_with_changes v{version_num}", - created=self.now, - created_by=None, ) # Soft-delete entity_with_pending_delete. After this, the Published @@ -385,15 +373,11 @@ def inner_reset_drafts_to_published(self, bulk: bool) -> None: new_entity = publishing_api.create_publishable_entity( self.learning_package_1.id, "new_entity", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( new_entity.id, version_num=1, title="I'm new_entity v1", - created=self.now, - created_by=None, ) # The versions we expect in (entity, published version_num, draft @@ -436,19 +420,16 @@ def inner_reset_drafts_to_published(self, bulk: bool) -> None: def test_get_entities_with_unpublished_changes(self) -> None: """Test fetching entities with unpublished changes after soft deletes.""" - entity = publishing_api.create_publishable_entity( - self.learning_package_1.id, - "my_entity", - created=self.now, - created_by=None, - ) - publishing_api.create_publishable_entity_version( - entity.id, - version_num=1, - title="An Entity 🌴", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(learning_package_id, None): + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity", + ) + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="An Entity 🌴", + ) # Fetch unpublished entities entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package_1.id) @@ -480,48 +461,39 @@ def test_filter_publishable_entities(self) -> None: count_drafts = 6 count_no_drafts = 3 - for index in range(count_published): - # Create entities to publish - entity = publishing_api.create_publishable_entity( - self.learning_package_1.id, - f"entity_published_{index}", - created=self.now, - created_by=None, - ) - - publishing_api.create_publishable_entity_version( - entity.id, - version_num=1, - title=f"Entity_published_{index}", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(learning_package_id, None): + for index in range(count_published): + # Create entities to publish + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + f"entity_published_{index}", + ) + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title=f"Entity_published_{index}", + ) publishing_api.publish_all_drafts(self.learning_package_1.id) - for index in range(count_drafts): - # Create entities with drafts - entity = publishing_api.create_publishable_entity( - self.learning_package_1.id, - f"entity_draft_{index}", - created=self.now, - created_by=None, - ) - - publishing_api.create_publishable_entity_version( - entity.id, - version_num=1, - title=f"Entity_draft_{index}", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(learning_package_id, None): + for index in range(count_drafts): + # Create entities with drafts + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + f"entity_draft_{index}", + ) + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title=f"Entity_draft_{index}", + ) for index in range(count_no_drafts): # Create entities without drafts entity = publishing_api.create_publishable_entity( self.learning_package_1.id, f"entity_no_draft_{index}", - created=self.now, created_by=None, ) @@ -588,52 +560,40 @@ def test_simple_draft_change_log(self) -> None: """ Simplest test that multiple writes make it into one DraftChangeLog. """ - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): entity = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity.id, version_num=1, title="An Entity 🌴", - created=self.now, - created_by=None, ) entity2 = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity2", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity2.id, version_num=1, title="An Entity 🌴 2", - created=self.now, - created_by=None, ) draft_sets = list(DraftChangeLog.objects.all()) assert len(draft_sets) == 1 assert len(draft_sets[0].records.all()) == 2 - # Now that we're outside of the context manager, check that we're making - # a new DraftChangeLog... - entity3 = publishing_api.create_publishable_entity( - self.learning_package_1.id, - "my_entity3", - created=self.now, - created_by=None, - ) - e3_v1 = publishing_api.create_publishable_entity_version( - entity3.id, - version_num=1, - title="An Entity 🌴 3", - created=self.now, - created_by=None, - ) + # And make one more version in a separate context manager + with publishing_api.draft_changes_for(self.learning_package_1.id, None): + entity3 = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity3", + ) + e3_v1 = publishing_api.create_publishable_entity_version( + entity3.id, + version_num=1, + title="An Entity 🌴 3", + ) draft_sets = list(DraftChangeLog.objects.all().order_by('id')) assert len(draft_sets) == 2 assert len(draft_sets[1].records.all()) == 1 @@ -650,21 +610,17 @@ def test_nested_draft_changesets(self) -> None: """ We should look up the stack to find the right one for our Learning Package. """ - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id) as dcl_1: + with publishing_api.draft_changes_for(self.learning_package_1.id, None) as dcl_1: lp1_e1 = publishing_api.create_publishable_entity( self.learning_package_1.id, "lp1_e1", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( lp1_e1.id, version_num=1, title="LP1 E1 v1", - created=self.now, - created_by=None, ) - with publishing_api.bulk_draft_changes_for(self.learning_package_2.id) as dcl_2: + with publishing_api.draft_changes_for(self.learning_package_2.id, None) as dcl_2: # This should make its way into the *outer* context, because # we're creating the new publishable entity version for # learning_package_1, not learning_package_2 @@ -672,8 +628,6 @@ def test_nested_draft_changesets(self) -> None: lp1_e1.id, version_num=2, title="LP1 E1 v1", - created=self.now, - created_by=None, ) # Make sure our change above made it to the outer context and @@ -690,15 +644,11 @@ def test_nested_draft_changesets(self) -> None: lp2_e1 = publishing_api.create_publishable_entity( self.learning_package_2.id, "lp2_e1", - created=self.now, - created_by=None, ) lp2_e1_v1 = publishing_api.create_publishable_entity_version( lp2_e1.id, version_num=1, title="LP2 E1 v1", - created=self.now, - created_by=None, ) # This doesn't error, but it creates a new DraftChangeLog instead of # re-using dcl_2 @@ -706,8 +656,6 @@ def test_nested_draft_changesets(self) -> None: lp2_e1.id, version_num=2, title="LP2 E1 v2", - created=self.now, - created_by=None, ) # Sanity check that the first/outer DraftChangeLog hasn't changed. @@ -735,26 +683,20 @@ def test_multiple_draft_changes(self) -> None: """ Test that multiple changes to the same entity are collapsed. """ - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): entity = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity.id, version_num=1, title="An Entity 🌴 v1", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity.id, version_num=2, title="An Entity 🌴 v2", - created=self.now, - created_by=None, ) draft_sets = list(DraftChangeLog.objects.all().order_by('id')) assert len(draft_sets) == 1 @@ -767,22 +709,18 @@ def test_multiple_draft_changes(self) -> None: def test_some_draft_changes_cancel_out(self) -> None: """Test that re remove redundant changes from our DraftChangeLog.""" - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): # This change will get cancelled out (because we create a draft and # then delete it), so changes related to entity_1 will be removed # after the context ends. entity_1 = publishing_api.create_publishable_entity( self.learning_package_1.id, "Entity-1", - created=self.now, - created_by=None, ) publishing_api.create_publishable_entity_version( entity_1.id, version_num=1, title="An Entity 🌴 v1", - created=self.now, - created_by=None, ) publishing_api.soft_delete_draft(entity_1.id) @@ -790,15 +728,11 @@ def test_some_draft_changes_cancel_out(self) -> None: entity_2 = publishing_api.create_publishable_entity( self.learning_package_1.id, "Entity-2", - created=self.now, - created_by=None, ) e2_v1 = publishing_api.create_publishable_entity_version( entity_2.id, version_num=1, title="E2 title", - created=self.now, - created_by=None, ) assert DraftChangeLog.objects.all().count() == 1 change_log = DraftChangeLog.objects.first() @@ -813,19 +747,15 @@ def test_multiple_draft_changes_all_cancel_out(self) -> None: If all changes made cancel out, the entire DraftRecord gets deleted. """ # Make sure a version change from (None -> None) gets removed. - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): entity = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity", - created=self.now, - created_by=None, ) v1 = publishing_api.create_publishable_entity_version( entity.id, version_num=1, title="An Entity 🌴 v1", - created=self.now, - created_by=None, ) publishing_api.soft_delete_draft(entity.id) @@ -836,15 +766,13 @@ def test_multiple_draft_changes_all_cancel_out(self) -> None: assert DraftChangeLog.objects.all().count() == 1 # Make sure a change from v1 -> v2 -> v1 gets removed. - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): for i in range(2, 5): # Make a few new versions publishing_api.create_publishable_entity_version( entity.id, version_num=i, title=f"An Entity v{i}", - created=self.now, - created_by=None, ) # Reset to version 1 publishing_api.set_draft_version(entity.id, v1.pk) @@ -878,32 +806,24 @@ def test_simple_publish_log(self) -> None: """ Simplest test that multiple writes make it into one PublishLog. """ - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): entity1 = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity", - created=self.now, - created_by=None, ) entity1_v1 = publishing_api.create_publishable_entity_version( entity1.id, version_num=1, title="An Entity 🌴", - created=self.now, - created_by=None, ) entity2 = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity2", - created=self.now, - created_by=None, ) entity2_v1 = publishing_api.create_publishable_entity_version( entity2.id, version_num=1, title="An Entity 🌴 2", - created=self.now, - created_by=None, ) # Check simplest publish of two things... @@ -1024,7 +944,7 @@ def setUpTestData(cls) -> None: created=cls.now, ) - with publishing_api.bulk_draft_changes_for(cls.learning_package_1.id): + with publishing_api.draft_changes_for(cls.learning_package_1.id): entity = publishing_api.create_publishable_entity( cls.learning_package_1.id, "my_entity", @@ -1849,7 +1769,7 @@ def test_parent_child_side_effects(self) -> None: def test_bulk_parent_child_side_effects(self) -> None: """Test that modifying a child has side-effects on its parent. (bulk version)""" - with publishing_api.bulk_draft_changes_for(self.learning_package.id): + with publishing_api.draft_changes_for(self.learning_package.id): child_1 = publishing_api.create_publishable_entity( self.learning_package.id, "child_1", @@ -1923,7 +1843,7 @@ def test_bulk_parent_child_side_effects(self) -> None: assert container_change.new_version == container_v1.publishable_entity_version # There are two side effects here, because we grouped our draft edits - # together using bulk_draft_changes_for, so changes to both children + # together using draft_changes_for, so changes to both children # count towards side-effects on the container. side_effects = DraftSideEffect.objects.all() assert side_effects.count() == 2 @@ -2049,7 +1969,7 @@ def test_multiple_layers_of_containers(self) -> None: # bottom-up using different DraftChangeLogs assert not DraftSideEffect.objects.all().exists() - with publishing_api.bulk_draft_changes_for(self.learning_package.id) as change_log: + with publishing_api.draft_changes_for(self.learning_package.id) as change_log: publishing_api.create_publishable_entity_version( component.id, version_num=2, @@ -2533,3 +2453,39 @@ def test_create_version_rejects_cross_package_dependencies(self) -> None: created_by=None, dependencies=[entity_in_lp2.id], ) + + def test_publish_functions_rejected_inside_draft_changes_for(self) -> None: + """ + publish_all_drafts() and publish_from_drafts() must not be callable + from within a draft_changes_for() context. + + draft_changes_for() opens a DraftChangeLog for accumulating draft + edits; running a publish inside it mixes draft-change bookkeeping with + publish bookkeeping in the same atomic block, which corrupts the + ordering of DraftChangeLog vs. PublishLog records and can leave Drafts + and Published rows out of sync if the outer context later raises. + """ + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "entity_for_bulk_publish_check", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="Entity v1", + created=self.now, + created_by=None, + ) + + with pytest.raises(ValidationError, match="Cannot publish while in draft_changes_for()."): + with publishing_api.draft_changes_for(self.learning_package_1.id): + publishing_api.publish_all_drafts(self.learning_package_1.id) + + with pytest.raises(ValidationError, match="Cannot publish while in draft_changes_for()."): + with publishing_api.draft_changes_for(self.learning_package_1.id): + publishing_api.publish_from_drafts( + self.learning_package_1.id, + Draft.objects.filter(entity__learning_package_id=self.learning_package_1.id), + ) diff --git a/tests/openedx_content/applets/publishing/test_signals.py b/tests/openedx_content/applets/publishing/test_signals.py index 405f933ab..b119717e1 100644 --- a/tests/openedx_content/applets/publishing/test_signals.py +++ b/tests/openedx_content/applets/publishing/test_signals.py @@ -248,7 +248,7 @@ def test_multiple_entites_changed(admin_user) -> None: api.create_publishable_entity_version(entity3.id, version_num=1, title="Entity 3 V1", **created_args) with capture_events(expected_count=1) as captured: - with api.bulk_draft_changes_for( + with api.draft_changes_for( learning_package.id, changed_by=admin_user.id, changed_at=now_time, @@ -298,7 +298,7 @@ def test_multiple_entites_change_aborted() -> None: with capture_events(expected_count=0): with abort_transaction(): - with api.bulk_draft_changes_for(learning_package.id, changed_by=None, changed_at=now_time): + with api.draft_changes_for(learning_package.id, changed_by=None, changed_at=now_time): # Note: the 'created_args' values below get ignored because of the bulk context. # Create two versions of entity1: api.create_publishable_entity_version(entity1.id, version_num=1, title="Entity 1 V1", **created_args) From 3c49de84bec0f4432a9d862914ec52f0decbafbe Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Thu, 30 Apr 2026 00:38:16 -0400 Subject: [PATCH 2/3] fix: Findings from Claude's audit --- .../applets/backup_restore/api.py | 1 + .../applets/backup_restore/zipper.py | 2 +- src/openedx_content/applets/components/api.py | 2 + src/openedx_content/applets/containers/api.py | 2 +- src/openedx_content/applets/publishing/api.py | 73 +++++++++---------- .../applets/publishing/test_api.py | 6 +- 6 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/openedx_content/applets/backup_restore/api.py b/src/openedx_content/applets/backup_restore/api.py index 8f880aa7b..49de268d5 100644 --- a/src/openedx_content/applets/backup_restore/api.py +++ b/src/openedx_content/applets/backup_restore/api.py @@ -8,6 +8,7 @@ from ..publishing.api import get_learning_package_by_ref from .zipper import LearningPackageUnzipper, LearningPackageZipper + # The public API that will be re-exported by openedx_content.api # is listed in the __all__ entries below. Internal helper functions that are # private to this module should start with an underscore. If a function does not diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 4b4d66525..b70c7ad41 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -780,7 +780,7 @@ def _save( self._save_subsections(learning_package_obj, containers) self._save_sections(learning_package_obj, containers) self._save_collections(learning_package_obj, collections) - publishing_api.publish_all_drafts(learning_package_obj.id) + publishing_api.publish_all_drafts(learning_package_obj.id, self.user) with publishing_api.draft_changes_for(learning_package_obj.id, self.user): self._save_draft_versions(components, containers, component_static_files) diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index a925a7376..77dde7ce4 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -90,6 +90,8 @@ def create_component( The ``entity_ref`` is conventionally derived as ``"{namespace}:{type_name}:{component_code}"``, although callers should not assume that this will always be true. + + You must specify `created_by=` unless you're inside a `draft_changes_for` context. """ entity_ref = f"{component_type.namespace}:{component_type.name}:{component_code}" with atomic(): diff --git a/src/openedx_content/applets/containers/api.py b/src/openedx_content/applets/containers/api.py index 31e61a84b..ad5670285 100644 --- a/src/openedx_content/applets/containers/api.py +++ b/src/openedx_content/applets/containers/api.py @@ -246,7 +246,7 @@ def _create_container_version( entity_list: EntityList, ) -> ContainerVersion: """ - Private internal method for logic shared juby create_container_version() and + Private internal method for logic shared by create_container_version() and create_next_container_version(). """ # validate entity_list using the type implementation: diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 2c3bb16e3..7235f1a32 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -48,6 +48,7 @@ "DATETIME_AUTO", "DatetimeOrAuto", "UserID", + "Author", "AUTHOR_AUTO", "AuthorOrAuto", "get_learning_package", @@ -120,25 +121,26 @@ """ -type AuthorOrAuto = ( - # Attribute to a specific user: - UserID | AbstractUser - # Attribue to nobody: - | None | AnonymousUser - # Attribute to the same user as the enclosing draft_changes_for: - | Literal["AUTHOR_AUTO"] -) +type Author = UserID | AbstractUser | AnonymousUser | None """ -How to attribute an operation (creation, edit, deletion, change) to a user. +A user attribution for a content operation (creation, edit, deletion, change). For attributable changes, this could be a User, or its database ID. For changes without any attributable author (e.g. backfills), this could be None or an AnonymousUser, both of which map to NULL in the database. This should be reserved for special cases--most changes have a concrete author! +""" + + +type AuthorOrAuto = Author | Literal["AUTHOR_AUTO"] +""" +Either an Author (which could be None) or AUTHOR_AUTO. -Finally, this could be AUTHOR_AUTO, which means "use the same author (or lack -thereof) that the draft change context is using." Most functions default to AUTHOR_AUTO. +Most functions default to AUTHOR_AUTO. Please note that `None` is not an +appopriate default author--only certain special operations are author-less; +by default, users should supply an author user or use AUTHOR_AUTO in order to +inherit the context's author user. """ @@ -147,7 +149,7 @@ def resolve_datetime( dt: DatetimeOrAuto, ) -> datetime: """ - Convert a Timestamp specifier into a concerete datetime to be saved to the DB. + Convert a Timestamp specifier into a concrete datetime to be saved to the DB. If `timestamp is DATETIME_AUTO`, then return either the datetime of the enclosing `draft_changes_for` context, or `datetime.now` if there is @@ -168,7 +170,7 @@ def resolve_author( author: AuthorOrAuto, ) -> UserID | None: """ - Convert a Author specifier into a concerete user ID (or None) to be saved to the DB. + Convert a Author specifier into a concrete user ID (or None) to be saved to the DB. If `author is AUTHOR_AUTO`, then return either the author of the enclosing `draft_changes_for` context. Raises `ValueError` if there is no @@ -184,7 +186,14 @@ def resolve_author( "An author (other than AUTHOR_AUTO) must be specified when changing content " "outside of a `draft_changes_for` context." ) - elif isinstance(author, AnonymousUser): + return _normalize_author_to_user_id(author) # type: ignore[arg-type] + + +def _normalize_author_to_user_id(author: Author) -> UserID | None: + """ + Given a user (object or ID) or lack thereof (AnonymousUser or None), return the ID or None. + """ + if isinstance(author, AnonymousUser): return None elif isinstance(author, AbstractUser): assert isinstance(author.pk, int) @@ -318,7 +327,7 @@ def create_publishable_entity( You'd typically want to call this right before creating your own content model that points to it. - Must be called inside `with draft_changes_for(...):` + You must specify `created_by=` unless you're inside a `draft_changes_for` context. """ return PublishableEntity.objects.create( @@ -520,9 +529,10 @@ def get_entities_with_unpublished_deletes(learning_package_id: LearningPackage.I def publish_all_drafts( learning_package_id: LearningPackage.ID, /, + published_by: Author, + *, message="", published_at: DatetimeOrAuto = DATETIME_AUTO, - published_by: AuthorOrAuto = AUTHOR_AUTO, ) -> PublishLog: """ Publish everything that is a Draft and is not already published. @@ -533,7 +543,11 @@ def publish_all_drafts( .with_unpublished_changes() ) return publish_from_drafts( - learning_package_id, draft_qset, message, published_at, published_by + learning_package_id, + draft_qset, + published_by=published_by, + message=message, + published_at=published_at, ) @@ -574,10 +588,10 @@ def publish_from_drafts( learning_package_id: LearningPackage.ID, /, draft_qset: QuerySet[Draft], + published_by: Author, + *, message: str = "", published_at: DatetimeOrAuto = DATETIME_AUTO, - published_by: AuthorOrAuto = AUTHOR_AUTO, - *, publish_dependencies: bool = True, ) -> PublishLog: """ @@ -589,7 +603,7 @@ def publish_from_drafts( if DraftChangeLogContext.get_active_draft_change_log(learning_package_id) is not None: raise ValidationError("Cannot publish while in draft_changes_for().") published_at = resolve_datetime(learning_package_id, published_at) - published_by = resolve_author(learning_package_id, published_by) + published_by = _normalize_author_to_user_id(published_by) with atomic(): if publish_dependencies: dependency_drafts_qsets = _get_dependencies_with_unpublished_changes(draft_qset) @@ -1015,13 +1029,6 @@ def set_draft_version( Calling this function attaches a new DraftChangeLogRecord and attaches it to a DraftChangeLog. - This function will create DraftSideEffect entries and properly add any - containers that may have been affected by this draft update, UNLESS it is - called from within a draft_changes_for block. If it is called from - inside a draft_changes_for block, it will not add side-effects for - containers, as draft_changes_for will automatically do that when the - block exits. @@TODO update this docstring - Must be called inside `with draft_changes_for(...):` """ with atomic(savepoint=False): @@ -1797,7 +1804,7 @@ def get_published_version_as_of( def draft_changes_for( learning_package_id: LearningPackage.ID, - changed_by: UserID | AbstractUser | AnonymousUser | None, + changed_by: Author, changed_at: DatetimeOrAuto = DATETIME_AUTO, ) -> DraftChangeLogContext: """ @@ -1824,15 +1831,7 @@ def draft_changes_for( else: assert isinstance(changed_at, datetime) changed_at_dt = changed_at - changed_by_id: UserID | None - if isinstance(changed_by, AnonymousUser): - changed_by_id = None - elif isinstance(changed_by, AbstractUser): - assert isinstance(changed_by.pk, int) - changed_by_id = changed_by.pk - elif changed_by: - assert isinstance(changed_by, int) - changed_by_id = changed_by + changed_by_id = _normalize_author_to_user_id(changed_by) return DraftChangeLogContext( learning_package_id, changed_at=changed_at_dt, diff --git a/tests/openedx_content/applets/publishing/test_api.py b/tests/openedx_content/applets/publishing/test_api.py index c38fa61e8..2051f9047 100644 --- a/tests/openedx_content/applets/publishing/test_api.py +++ b/tests/openedx_content/applets/publishing/test_api.py @@ -420,7 +420,7 @@ def inner_reset_drafts_to_published(self, bulk: bool) -> None: def test_get_entities_with_unpublished_changes(self) -> None: """Test fetching entities with unpublished changes after soft deletes.""" - with publishing_api.draft_changes_for(learning_package_id, None): + with publishing_api.draft_changes_for(learning_package.id, None): entity = publishing_api.create_publishable_entity( self.learning_package_1.id, "my_entity", @@ -461,7 +461,7 @@ def test_filter_publishable_entities(self) -> None: count_drafts = 6 count_no_drafts = 3 - with publishing_api.draft_changes_for(learning_package_id, None): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): for index in range(count_published): # Create entities to publish entity = publishing_api.create_publishable_entity( @@ -476,7 +476,7 @@ def test_filter_publishable_entities(self) -> None: publishing_api.publish_all_drafts(self.learning_package_1.id) - with publishing_api.draft_changes_for(learning_package_id, None): + with publishing_api.draft_changes_for(self.learning_package_1.id, None): for index in range(count_drafts): # Create entities with drafts entity = publishing_api.create_publishable_entity( From eb255b2f522cb51e688382998558eddd76cff1df Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Thu, 30 Apr 2026 01:19:02 -0400 Subject: [PATCH 3/3] test: Fix some of the tests Co-Authored-By: Claude Opus 4.7 1M Context Co-Authored-By: Claude Sonnet 4.6 --- .../applets/backup_restore/test_backup.py | 138 +++--- .../applets/collections/test_api.py | 31 +- .../applets/components/test_api.py | 348 +++++++-------- .../applets/components/test_assets.py | 25 +- .../applets/components/test_models.py | 70 ++- .../applets/containers/test_api.py | 418 +++++++++--------- .../applets/sections/test_api.py | 110 +++-- .../applets/subsections/test_api.py | 69 ++- .../openedx_content/applets/units/test_api.py | 74 ++-- 9 files changed, 620 insertions(+), 663 deletions(-) diff --git a/tests/openedx_content/applets/backup_restore/test_backup.py b/tests/openedx_content/applets/backup_restore/test_backup.py index 1c9e5cf8b..46110ecba 100644 --- a/tests/openedx_content/applets/backup_restore/test_backup.py +++ b/tests/openedx_content/applets/backup_restore/test_backup.py @@ -65,30 +65,28 @@ def setUpTestData(cls): cls.html_type = api.get_or_create_component_type(cls.xblock_v1_namespace, "html") cls.problem_type = api.get_or_create_component_type(cls.xblock_v1_namespace, "problem") - # Make and publish one Component - cls.published_component, _ = api.create_component_and_version( - cls.learning_package.id, - cls.problem_type, - component_code="my_published_example", - title="My published problem", - created=cls.now, - created_by=cls.user.id, - ) + with api.draft_changes_for(cls.learning_package.id, cls.user): + # Make and publish one Component + cls.published_component, _ = api.create_component_and_version( + cls.learning_package.id, + cls.problem_type, + component_code="my_published_example", + title="My published problem", + ) - # Make and publish one Component - # Same local key as above to test uniqueness of slugified hash - cls.published_component2, _ = api.create_component_and_version( - cls.learning_package.id, - cls.problem_type, - component_code="My_published_example", - title="My published problem 2", - created=cls.now, - created_by=cls.user.id, - ) + # Make and publish one Component + # Same local key as above to test uniqueness of slugified hash + cls.published_component2, _ = api.create_component_and_version( + cls.learning_package.id, + cls.problem_type, + component_code="My_published_example", + title="My published problem 2", + ) # Create a Content entry for the published Component api.publish_all_drafts( cls.learning_package.id, + cls.user, message="Publish from CollectionTestCase.setUpTestData", published_at=cls.now, ) @@ -99,24 +97,22 @@ def setUpTestData(cls): text="This is some data", created=cls.now, ) - api.create_next_component_version( - cls.published_component.id, - title="My published problem draft v2", - media_to_replace={ - 'hello.txt': new_txt_media - }, - created=cls.now, - ) + with api.draft_changes_for(cls.learning_package.id, cls.user): + api.create_next_component_version( + cls.published_component.id, + title="My published problem draft v2", + media_to_replace={ + 'hello.txt': new_txt_media + }, + ) - # Create a Draft component, one in each learning package - cls.draft_component, _ = api.create_component_and_version( - cls.learning_package.id, - cls.html_type, - component_code="my_draft_example", - title="My draft html", - created=cls.now, - created_by=cls.user.id, - ) + # Create a Draft component, one in each learning package + cls.draft_component, _ = api.create_component_and_version( + cls.learning_package.id, + cls.html_type, + component_code="my_draft_example", + title="My draft html", + ) cls.html_asset_media = api.get_or_create_file_media( cls.learning_package.id, @@ -124,39 +120,36 @@ def setUpTestData(cls): data=b"hello world!", created=cls.now, ) - api.create_next_component_version( - cls.draft_component.id, - title="My draft html v2", - media_to_replace={ - "static/other/subdirectory/hello.html": cls.html_asset_media - }, - created=cls.now, - ) + with api.draft_changes_for(cls.learning_package.id, cls.user): + api.create_next_component_version( + cls.draft_component.id, + title="My draft html v2", + media_to_replace={ + "static/other/subdirectory/hello.html": cls.html_asset_media + }, + ) - components = api.get_publishable_entities(cls.learning_package) - cls.all_components = components + components = api.get_publishable_entities(cls.learning_package) + cls.all_components = components - cls.collection = api.create_collection( - cls.learning_package.id, - collection_code="COL1", - created_by=cls.user.id, - title="Collection 1", - description="Description of Collection 1", - ) + cls.collection = api.create_collection( + cls.learning_package.id, + collection_code="COL1", + title="Collection 1", + description="Description of Collection 1", + ) - api.add_to_collection( - cls.learning_package.id, - cls.collection.collection_code, - components - ) + api.add_to_collection( + cls.learning_package.id, + cls.collection.collection_code, + components + ) - api.create_container( - learning_package_id=cls.learning_package.id, - container_code="unit-1", - created=cls.now, - created_by=cls.user.id, - container_cls=Unit, - ) + api.create_container( + learning_package_id=cls.learning_package.id, + container_code="unit-1", + container_cls=Unit, + ) def check_toml_file(self, zip_path: Path, zip_member_name: Path, content_to_check: list): """ @@ -304,14 +297,13 @@ def test_queries_n_plus_problem(self): list(entities) # force evaluation self.assertEqual(len(entities), 4) # Add another component - api.create_component_and_version( - self.learning_package.id, - self.problem_type, - component_code="my_published_example2", - title="My published problem 2", - created=self.now, - created_by=self.user.id, - ) + with api.draft_changes_for(self.learning_package.id, self.user): + api.create_component_and_version( + self.learning_package.id, + self.problem_type, + component_code="my_published_example2", + title="My published problem 2", + ) entities = zipper.get_publishable_entities() with self.assertNumQueries(3): list(entities) # force evaluation diff --git a/tests/openedx_content/applets/collections/test_api.py b/tests/openedx_content/applets/collections/test_api.py index 76d234dc2..04d5de8bd 100644 --- a/tests/openedx_content/applets/collections/test_api.py +++ b/tests/openedx_content/applets/collections/test_api.py @@ -278,29 +278,28 @@ def setUpTestData(cls) -> None: ) # Make and publish one Component - cls.published_component, _ = api.create_component_and_version( - cls.learning_package.id, - cls.problem_type, - component_code="my_published_example", - title="My published problem", - created=cls.now, - created_by=cls.user.id, - ) + with api.draft_changes_for(cls.learning_package.id, cls.user): + cls.published_component, _ = api.create_component_and_version( + cls.learning_package.id, + cls.problem_type, + component_code="my_published_example", + title="My published problem", + ) api.publish_all_drafts( cls.learning_package.id, + cls.user, message="Publish from CollectionTestCase.setUpTestData", published_at=cls.now, ) # Create a Draft component, one in each learning package - cls.draft_component, _ = api.create_component_and_version( - cls.learning_package.id, - cls.html_type, - component_code="my_draft_example", - title="My draft html", - created=cls.now, - created_by=cls.user.id, - ) + with api.draft_changes_for(cls.learning_package.id, cls.user): + cls.draft_component, _ = api.create_component_and_version( + cls.learning_package.id, + cls.html_type, + component_code="my_draft_example", + title="My draft html", + ) # Add some shared components to the collections cls.collection1 = api.add_to_collection( diff --git a/tests/openedx_content/applets/components/test_api.py b/tests/openedx_content/applets/components/test_api.py index b56eb67af..0971790c6 100644 --- a/tests/openedx_content/applets/components/test_api.py +++ b/tests/openedx_content/applets/components/test_api.py @@ -53,20 +53,20 @@ def publish_component(self, component: Component): draft_qset=publishing_api.get_all_drafts(self.learning_package.id).filter( entity=component.publishable_entity, ), + published_by=None, ) def create_component(self, *, title: str = "Test Component", component_code: str = "component_1") -> tuple[ Component, ComponentVersion ]: """ Helper method to quickly create a component """ - return components_api.create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code=component_code, - title=title, - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + return components_api.create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code=component_code, + title=title, + ) class PerformanceTestCase(ComponentTestCase): @@ -84,17 +84,17 @@ def test_component_num_queries(self) -> None: """ Create a basic component and test that we fetch it back in 1 query. """ - component, _version = components_api.create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="Query_Counting", - title="Querying Counting Problem", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + component, _version = components_api.create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="Query_Counting", + title="Querying Counting Problem", + ) publishing_api.publish_all_drafts( self.learning_package.id, - published_at=self.now + published_by=None, + published_at=self.now, ) # We should be fetching all of this with a select-related, so only one @@ -129,56 +129,49 @@ def setUpTestData(cls) -> None: super().setUpTestData() v2_problem_type = components_api.get_or_create_component_type("xblock.v2", "problem") - cls.published_problem, _version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=v2_problem_type, - component_code="pp_lk", - title="Published Problem", - created=cls.now, - created_by=None, - ) - cls.published_html, _version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=cls.html_type, - component_code="ph_lk", - title="Published HTML", - created=cls.now, - created_by=None, - ) + with publishing_api.draft_changes_for(cls.learning_package.id, None): + cls.published_problem, _version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=v2_problem_type, + component_code="pp_lk", + title="Published Problem", + ) + cls.published_html, _version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=cls.html_type, + component_code="ph_lk", + title="Published HTML", + ) publishing_api.publish_all_drafts( cls.learning_package.id, - published_at=cls.now + published_by=None, + published_at=cls.now, ) - # Components that exist only as Drafts - cls.unpublished_problem, _version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=v2_problem_type, - component_code="upp_lk", - title="Unpublished Problem", - created=cls.now, - created_by=None, - ) - cls.unpublished_html, _version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=cls.html_type, - component_code="uph_lk", - title="Unpublished HTML", - created=cls.now, - created_by=None, - ) + with publishing_api.draft_changes_for(cls.learning_package.id, None): + # Components that exist only as Drafts + cls.unpublished_problem, _version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=v2_problem_type, + component_code="upp_lk", + title="Unpublished Problem", + ) + cls.unpublished_html, _version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=cls.html_type, + component_code="uph_lk", + title="Unpublished HTML", + ) - # Component we're putting here to soft delete (this will remove the - # Draft entry) - cls.deleted_video, _version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=cls.video_type, - component_code="dv_lk", - title="Deleted Video", - created=cls.now, - created_by=None, - ) - publishing_api.soft_delete_draft(cls.deleted_video.id) + # Component we're putting here to soft delete (this will remove the + # Draft entry) + cls.deleted_video, _version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=cls.video_type, + component_code="dv_lk", + title="Deleted Video", + ) + publishing_api.soft_delete_draft(cls.deleted_video.id) def test_no_filters(self): """ @@ -444,16 +437,15 @@ def test_add(self): text="This is some data", created=self.now, ) - components_api.create_component_version( - self.problem.id, - version_num=1, - title="My Title", - created=self.now, - created_by=None, - media={ - "my/path/to/hello.txt": new_media - } - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + components_api.create_component_version( + self.problem.id, + version_num=1, + title="My Title", + media={ + "my/path/to/hello.txt": new_media + } + ) # re-fetch from the database to check to see if we wrote it correctly new_version = components_api.get_component(self.problem.id) \ @@ -473,13 +465,13 @@ def test_add(self): ] for invalid_path in invalid_paths: with self.assertRaises(ValueError): - new_version = components_api.create_next_component_version( - self.problem.id, - media_to_replace={ - invalid_path: new_media - }, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + new_version = components_api.create_next_component_version( + self.problem.id, + media_to_replace={ + invalid_path: new_media + }, + ) def test_create_with_media_or_id(self): """Test that we can create Media associations with Media or Media.ID.""" @@ -489,18 +481,18 @@ def test_create_with_media_or_id(self): data=b"print('hello world!')", created=self.now, ) - _problem, version = components_api.create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="media_problem_1", - title="Testing Media Associations", - created=self.now, - media={ - 'hello.py': python_src_media, - 'hello2.py': python_src_media.id, - 'hello.txt': b"Bytes are tested in test_bytes_content()" - } - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + _problem, version = components_api.create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="media_problem_1", + title="Testing Media Associations", + media={ + 'hello.py': python_src_media, + 'hello2.py': python_src_media.id, + 'hello.txt': b"Bytes are tested in test_bytes_content()" + } + ) assert ( python_src_media == version.media.get(componentversionmedia__path="hello.py") == @@ -509,16 +501,16 @@ def test_create_with_media_or_id(self): # But we don't accept None as a media value with self.assertRaises(ValueError): - components_api.create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="media_problem_2", - title="This shouldn't work", - created=self.now, - media={ - "block.xml": None - } - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + components_api.create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="media_problem_2", + title="This shouldn't work", + media={ + "block.xml": None + } + ) # We also don't allow a different LearningPackage's Media to be assigned different_lp = publishing_api.create_learning_package( @@ -531,29 +523,29 @@ def test_create_with_media_or_id(self): created=self.now, ) with self.assertRaises(ValueError): - components_api.create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="media_problem_3", - title="This also shouldn't work", - created=self.now, - media={ - "invalid-outside-media.txt": outside_media, - } - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + components_api.create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="media_problem_3", + title="This also shouldn't work", + media={ + "invalid-outside-media.txt": outside_media, + } + ) def test_bytes_content(self): bytes_media = b'raw content' - version_1 = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 1", - media_to_replace={ - "raw.txt": bytes_media, - "no_ext": bytes_media, - }, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_1 = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 1", + media_to_replace={ + "raw.txt": bytes_media, + "no_ext": bytes_media, + }, + ) content_txt = version_1.media.get(componentversionmedia__path="raw.txt") content_raw_txt = version_1.media.get(componentversionmedia__path="no_ext") @@ -587,15 +579,15 @@ def test_multiple_versions(self): ) # Two text files, hello.txt and goodbye.txt - version_1 = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 1", - media_to_replace={ - "hello.txt": hello_media.pk, - "goodbye.txt": goodbye_media.pk, - }, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_1 = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 1", + media_to_replace={ + "hello.txt": hello_media.pk, + "goodbye.txt": goodbye_media.pk, + }, + ) assert version_1.version_num == 1 assert version_1.title == "Problem Version 1" version_1_contents = list(version_1.media.all()) @@ -613,15 +605,15 @@ def test_multiple_versions(self): # This should keep the old value for goodbye.txt, add blank.txt, and set # hello.txt to be a new value (blank). - version_2 = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 2", - media_to_replace={ - "hello.txt": blank_media.pk, - "blank.txt": blank_media.pk, - }, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_2 = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 2", + media_to_replace={ + "hello.txt": blank_media.pk, + "blank.txt": blank_media.pk, + }, + ) assert version_2.version_num == 2 assert version_2.media.count() == 3 assert ( @@ -642,17 +634,17 @@ def test_multiple_versions(self): # Now we're going to set "hello.txt" back to hello_content, but remove # blank.txt, goodbye.txt, and an unknown "nothere.txt". - version_3 = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 3", - media_to_replace={ - "hello.txt": hello_media.pk, - "blank.txt": None, - "goodbye.txt": None, - "nothere.txt": None, # should not error - }, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_3 = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 3", + media_to_replace={ + "hello.txt": hello_media.pk, + "blank.txt": None, + "goodbye.txt": None, + "nothere.txt": None, # should not error + }, + ) assert version_3.version_num == 3 assert version_3.media.count() == 1 assert ( @@ -663,13 +655,13 @@ def test_multiple_versions(self): def test_create_next_version_forcing_num_version(self): """Test creating a next version with a forced version number.""" - version_1 = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 1", - media_to_replace={}, - created=self.now, - force_version_num=5, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_1 = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 1", + media_to_replace={}, + force_version_num=5, + ) assert version_1.version_num == 5 def test_create_multiple_next_versions_and_diff_content(self): @@ -695,26 +687,27 @@ def test_create_multiple_next_versions_and_diff_content(self): 'static/profile.webp': python_source_asset.pk, 'static/new_file.webp': python_source_asset.pk, } - version_1_published = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 1", - media_to_replace=media_to_replace_for_published, - created=self.now, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_1_published = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 1", + media_to_replace=media_to_replace_for_published, + ) assert version_1_published.version_num == 1 publishing_api.publish_all_drafts( self.learning_package.id, - published_at=self.now + published_by=None, + published_at=self.now, ) - version_2_draft = components_api.create_next_component_version( - self.problem.id, - title="Problem Version 2", - media_to_replace=media_to_replace_for_draft, - created=self.now, - ignore_previous_media=True, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + version_2_draft = components_api.create_next_component_version( + self.problem.id, + title="Problem Version 2", + media_to_replace=media_to_replace_for_draft, + ignore_previous_media=True, + ) assert version_2_draft.version_num == 2 assert version_2_draft.media.count() == 2 assert ( @@ -750,14 +743,13 @@ def setUpTestData(cls) -> None: """ super().setUpTestData() v2_problem_type = components_api.get_or_create_component_type("xblock.v2", "problem") - cls.published_problem, _ = components_api.create_component_and_version( - cls.learning_package.id, - component_type=v2_problem_type, - component_code="pp_lk", - title="Published Problem", - created=cls.now, - created_by=None, - ) + with publishing_api.draft_changes_for(cls.learning_package.id, None): + cls.published_problem, _ = components_api.create_component_and_version( + cls.learning_package.id, + component_type=v2_problem_type, + component_code="pp_lk", + title="Published Problem", + ) cls.collection1 = collection_api.create_collection( cls.learning_package.id, collection_code="MYCOL1", diff --git a/tests/openedx_content/applets/components/test_assets.py b/tests/openedx_content/applets/components/test_assets.py index 27a37abee..cd9f3619d 100644 --- a/tests/openedx_content/applets/components/test_assets.py +++ b/tests/openedx_content/applets/components/test_assets.py @@ -79,19 +79,18 @@ def setUpTestData(cls) -> None: data=b"hello world!", created=cls.now, ) - cls.component, cls.component_version = components_api.create_component_and_version( - cls.learning_package.id, - component_type=cls.problem_type, - component_code="my_problem", - title="My Problem", - created=cls.now, - created_by=None, - media={ - "block.xml": cls.problem_media, - "src/grader.py": cls.python_source_asset, - "static/hello.html": cls.html_asset_media - } - ) + with publishing_api.draft_changes_for(cls.learning_package.id, None): + cls.component, cls.component_version = components_api.create_component_and_version( + cls.learning_package.id, + component_type=cls.problem_type, + component_code="my_problem", + title="My Problem", + media={ + "block.xml": cls.problem_media, + "src/grader.py": cls.python_source_asset, + "static/hello.html": cls.html_asset_media + } + ) def test_no_component_version(self): """No ComponentVersion matching the UUID exists.""" diff --git a/tests/openedx_content/applets/components/test_models.py b/tests/openedx_content/applets/components/test_models.py index e029bed17..e9e3d0e73 100644 --- a/tests/openedx_content/applets/components/test_models.py +++ b/tests/openedx_content/applets/components/test_models.py @@ -17,6 +17,7 @@ LearningPackage, create_learning_package, create_publishable_entity_version, + draft_changes_for, publish_all_drafts, ) @@ -48,17 +49,16 @@ def setUpTestData(cls) -> None: # Note: we must specify '-> None' to opt in to cls.problem_type = get_or_create_component_type("xblock.v1", "problem") def test_latest_version(self) -> None: - component, component_version = create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="monty_hall", - title="Monty Hall Problem", - created=self.now, - created_by=None, - ) + with draft_changes_for(self.learning_package.id, None): + component, component_version = create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="monty_hall", + title="Monty Hall Problem", + ) assert component.versioning.draft == component_version assert component.versioning.published is None - publish_all_drafts(self.learning_package.id, published_at=self.now) + publish_all_drafts(self.learning_package.id, published_by=None, published_at=self.now) # Publishing isn't immediately reflected in the component obj (it's # using a cached version). @@ -78,30 +78,27 @@ def test_last_publish_log(self): """ Test last_publish_log versioning property for published Components. """ - # This Component will get a couple of Published versions - component_with_changes, _ = create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="with_changes", - title="Component with changes v1", - created=self.now, - created_by=None, - ) - - # This Component will only be Published once. - component_with_no_changes, _ = create_component_and_version( - self.learning_package.id, - component_type=self.problem_type, - component_code="with_no_changes", - title="Component with no changes v1", - created=self.now, - created_by=None, - ) + with draft_changes_for(self.learning_package.id, None): + # This Component will get a couple of Published versions + component_with_changes, _ = create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="with_changes", + title="Component with changes v1", + ) + + # This Component will only be Published once. + component_with_no_changes, _ = create_component_and_version( + self.learning_package.id, + component_type=self.problem_type, + component_code="with_no_changes", + title="Component with no changes v1", + ) # Publish first time. published_first_time = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) with freeze_time(published_first_time): - publish_all_drafts(self.learning_package.id) + publish_all_drafts(self.learning_package.id, published_by=None) # Refetch the entities to get latest versions component_with_changes = get_component(component_with_changes.id) @@ -119,18 +116,17 @@ def test_last_publish_log(self): ) # Modify component_with_changes - create_publishable_entity_version( - component_with_changes.publishable_entity.id, - version_num=2, - title="Component with changes v2", - created=self.now, - created_by=None, - ) + with draft_changes_for(self.learning_package.id, None): + create_publishable_entity_version( + component_with_changes.publishable_entity.id, + version_num=2, + title="Component with changes v2", + ) # Publish second time published_second_time = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) with freeze_time(published_second_time): - publish_all_drafts(self.learning_package.id) + publish_all_drafts(self.learning_package.id, published_by=None) # Refetch the entities to get latest versions component_with_changes = get_component(component_with_changes.id) diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index fe99d8bc5..94cdf55f0 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -105,14 +105,13 @@ def create_test_entity(learning_package: LearningPackage, ref: str, title: str) """Create a TestEntity with a draft version""" pe = publishing_api.create_publishable_entity(learning_package.id, ref, created=now, created_by=None) new_entity = TestEntity.objects.create(publishable_entity=pe) - pev = publishing_api.create_publishable_entity_version( - new_entity.pk, - version_num=1, - title=title, - created=now, - created_by=None, - ) - TestEntityVersion.objects.create(publishable_entity_version=pev) + with publishing_api.draft_changes_for(learning_package.id, None): + pev = publishing_api.create_publishable_entity_version( + new_entity.pk, + version_num=1, + title=title, + ) + TestEntityVersion.objects.create(publishable_entity_version=pev) return new_entity @@ -147,15 +146,14 @@ def create_test_container( title: str = "", ) -> TestContainer: """Create a TestContainer with a draft version""" - container, _version = containers_api.create_container_and_version( - learning_package.id, - container_code=container_code, - title=title or f"Container ({container_code})", - entities=entities, - container_cls=TestContainer, - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(learning_package.id, None): + container, _version = containers_api.create_container_and_version( + learning_package.id, + container_code=container_code, + title=title or f"Container ({container_code})", + entities=entities, + container_cls=TestContainer, + ) return container @@ -217,15 +215,14 @@ def _grandparent( parent_of_three: TestContainer, ) -> ContainerContainer: """An ContainerContainer with two unpinned children""" - grandparent, _version = containers_api.create_container_and_version( - lp.id, - container_code="grandparent", - title="Generic Container with Two Unpinned TestContainer children", - entities=[parent_of_two, parent_of_three], - container_cls=ContainerContainer, - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + grandparent, _version = containers_api.create_container_and_version( + lp.id, + container_code="grandparent", + title="Generic Container with Two Unpinned TestContainer children", + entities=[parent_of_two, parent_of_three], + container_cls=ContainerContainer, + ) return grandparent @@ -236,14 +233,14 @@ def _container_of_uninstalled_type(lp: LearningPackage, child_entity1: TestEntit e.g. leftover data from an uninstalled plugin. """ # First create a TestContainer, then we'll modify it to simulate it being from an uninstalled plugin - container, _ = containers_api.create_container_and_version( - lp.id, - container_code="abandoned-container", - title="Abandoned Container 1", - entities=[child_entity1], - container_cls=TestContainer, - created=now, - ) + with publishing_api.draft_changes_for(lp.id, None): + container, _ = containers_api.create_container_and_version( + lp.id, + container_code="abandoned-container", + title="Abandoned Container 1", + entities=[child_entity1], + container_cls=TestContainer, + ) # Now create the plugin type (no public API for this; only do this in a test) ctr = ContainerType.objects.create(type_code="misc") Container.objects.filter(pk=container.id).update(container_type=ctr) @@ -253,15 +250,14 @@ def _container_of_uninstalled_type(lp: LearningPackage, child_entity1: TestEntit @pytest.fixture(name="other_lp_parent") def _other_lp_parent(lp2: LearningPackage, other_lp_child: TestEntity) -> TestContainer: """An TestContainer with one child""" - other_lp_parent, _version = containers_api.create_container_and_version( - lp2.id, - container_code="other_lp_parent", - title="Generic Container with One Unpinned Child Entity", - entities=[other_lp_child], - container_cls=TestContainer, - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp2.id, None): + other_lp_parent, _version = containers_api.create_container_and_version( + lp2.id, + container_code="other_lp_parent", + title="Generic Container with One Unpinned Child Entity", + entities=[other_lp_child], + container_cls=TestContainer, + ) return other_lp_parent @@ -271,16 +267,19 @@ def publish_entity(obj: PublishableEntityMixin) -> PublishLog: return publishing_api.publish_from_drafts( lp_id, draft_qset=publishing_api.get_all_drafts(lp_id).filter(entity=obj.publishable_entity), + published_by=None, ) def modify_entity(obj: TestEntity, title="Newly modified entity"): """Modify a TestEntity, creating a new version with a new title""" assert isinstance(obj, TestEntity) - new_raw_version = publishing_api.create_publishable_entity_version( - obj.pk, version_num=obj.versioning.latest.version_num + 1, title=title, created=now, created_by=None - ) - return TestEntityVersion.objects.create(pk=new_raw_version.pk) + lp_id = obj.publishable_entity.learning_package_id + with publishing_api.draft_changes_for(lp_id, None): + new_raw_version = publishing_api.create_publishable_entity_version( + obj.pk, version_num=obj.versioning.latest.version_num + 1, title=title + ) + return TestEntityVersion.objects.create(pk=new_raw_version.pk) def Entry( @@ -303,15 +302,14 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None """ Creating an empty TestContainer. It will have only a draft version. """ - container, container_v1 = containers_api.create_container_and_version( - lp.id, - container_code="new-container-1", - title="Test Container 1", - container_cls=TestContainer, - created=now, - created_by=admin_user.pk, - can_stand_alone=False, - ) + with publishing_api.draft_changes_for(lp.id, admin_user, changed_at=now): + container, container_v1 = containers_api.create_container_and_version( + lp.id, + container_code="new-container-1", + title="Test Container 1", + container_cls=TestContainer, + can_stand_alone=False, + ) if TYPE_CHECKING: # `create_container_and_version()` should return the correct Container subclass, based on `container_cls=...`: @@ -352,14 +350,13 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None def test_unicode_code(lp: LearningPackage) -> None: """container_code supports non-ascii letters.""" unicode_code = "柏倉隆史" - container, _ = containers_api.create_container_and_version( - lp.id, - container_code=unicode_code, - title="Unicode Container", - container_cls=TestContainer, - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + container, _ = containers_api.create_container_and_version( + lp.id, + container_code=unicode_code, + title="Unicode Container", + container_cls=TestContainer, + ) assert container.container_code == unicode_code assert containers_api.get_container_by_code(lp.id, unicode_code).id == container.id @@ -368,16 +365,15 @@ def test_create_container_queries(lp: LearningPackage, child_entity1: TestEntity """Test how many database queries are required to create a container.""" base_args: dict[str, Any] = { "title": "Test Container", - "created": now, - "created_by": None, "container_cls": TestContainer, } # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with django_assert_num_queries(34): - containers_api.create_container_and_version(lp.id, container_code="c1", **base_args) - # And try with a a container that has children: - with django_assert_num_queries(37): - containers_api.create_container_and_version(lp.id, container_code="c2", **base_args, entities=[child_entity1]) + with publishing_api.draft_changes_for(lp.id, None): + with django_assert_num_queries(34): + containers_api.create_container_and_version(lp.id, container_code="c1", **base_args) + # And try with a a container that has children: + with django_assert_num_queries(37): + containers_api.create_container_and_version(lp.id, container_code="c2", **base_args, entities=[child_entity1]) # versioning helpers @@ -403,7 +399,7 @@ def test_container_versioning_helpers(parent_of_two: TestContainer): # create_next_container_version -def test_create_next_container_version_no_changes(parent_of_two: TestContainer, other_user): +def test_create_next_container_version_no_changes(lp: LearningPackage, parent_of_two: TestContainer, other_user): """ Test creating a new version of the "parent of two" container, but without any actual changes. @@ -413,12 +409,11 @@ def test_create_next_container_version_no_changes(parent_of_two: TestContainer, # Create a new version with no changes: v2_date = datetime.now(tz=timezone.utc) - version_2 = containers_api.create_next_container_version( - parent_of_two, - created=v2_date, - created_by=other_user.pk, - # Specify no changes at all - ) + with publishing_api.draft_changes_for(lp.id, other_user, changed_at=v2_date): + version_2 = containers_api.create_next_container_version( + parent_of_two, + # Specify no changes at all + ) # assert_type(version_2, TestContainerVersion) # ^ Must come before 'assert isinstance(...)'. Unfortunately, getting the subclass return type in python 3.12 is not @@ -441,7 +436,7 @@ def test_create_next_container_version_no_changes(parent_of_two: TestContainer, def test_create_next_container_version_with_changes( - parent_of_two: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity + lp: LearningPackage, parent_of_two: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity ): """ Test creating a new version of the "parent of two" container, changing the @@ -452,14 +447,13 @@ def test_create_next_container_version_with_changes( # Create a new version, specifying version number 5 and changing the title and the order of the children: v5_date = datetime.now(tz=timezone.utc) - containers_api.create_next_container_version( - parent_of_two, - title="New Title - children reversed", - entities=[child_entity2, child_entity1], # Reversed from original [child_entity1, child_entity2] order - force_version_num=5, - created=v5_date, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None, changed_at=v5_date): + containers_api.create_next_container_version( + parent_of_two, + title="New Title - children reversed", + entities=[child_entity2, child_entity1], # Reversed from original [child_entity1, child_entity2] order + force_version_num=5, + ) # Now retrieve the new version: version_5 = parent_of_two.versioning.draft @@ -473,6 +467,7 @@ def test_create_next_container_version_with_changes( def test_create_next_container_version_with_append( + lp: LearningPackage, parent_of_two: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -487,13 +482,12 @@ def test_create_next_container_version_with_append( assert child_entity1_v1.version_num == 1 # Create a new version, APPENDing entity 3 and 📌 pinned entity1 (v1) - version_2 = containers_api.create_next_container_version( - parent_of_two, - entities=[child_entity3, child_entity1_v1], - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.APPEND, - ) + with publishing_api.draft_changes_for(lp.id, None): + version_2 = containers_api.create_next_container_version( + parent_of_two, + entities=[child_entity3, child_entity1_v1], + entities_action=containers_api.ChildrenEntitiesAction.APPEND, + ) assert parent_of_two.versioning.draft == version_2 assert containers_api.get_entities_in_container(parent_of_two, published=False) == [ @@ -505,6 +499,7 @@ def test_create_next_container_version_with_append( def test_create_next_container_version_with_remove_1( + lp: LearningPackage, parent_of_six: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -523,13 +518,12 @@ def test_create_next_container_version_with_remove_1( Entry(child_entity3.versioning.draft, pinned=False), # entity 3, unpinned ] # Remove "entity 1 unpinned" - should remove both: - containers_api.create_next_container_version( - parent_of_six.id, - entities=[child_entity1], - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.REMOVE, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_six.id, + entities=[child_entity1], + entities_action=containers_api.ChildrenEntitiesAction.REMOVE, + ) # Now it looks like: assert containers_api.get_entities_in_container(parent_of_six, published=False) == [ @@ -542,6 +536,7 @@ def test_create_next_container_version_with_remove_1( def test_create_next_container_version_with_remove_2( + lp: LearningPackage, parent_of_six: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -560,13 +555,12 @@ def test_create_next_container_version_with_remove_2( Entry(child_entity3.versioning.draft, pinned=False), # entity 3, unpinned ] # Remove "entity 2 pinned" - should remove both: - containers_api.create_next_container_version( - parent_of_six.id, - entities=[child_entity2.versioning.draft], # specify the version for "pinned" - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.REMOVE, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_six.id, + entities=[child_entity2.versioning.draft], # specify the version for "pinned" + entities_action=containers_api.ChildrenEntitiesAction.REMOVE, + ) # Now it looks like: assert containers_api.get_entities_in_container(parent_of_six, published=False) == [ @@ -580,6 +574,7 @@ def test_create_next_container_version_with_remove_2( def test_create_next_container_version_with_remove_3( + lp: LearningPackage, parent_of_six: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -598,13 +593,12 @@ def test_create_next_container_version_with_remove_3( Entry(child_entity3.versioning.draft, pinned=False), # entity 3, unpinned ] # Remove "entity 3 pinned" - should remove only one: - containers_api.create_next_container_version( - parent_of_six.id, - entities=[child_entity3.versioning.draft], # specify the version for "pinned" - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.REMOVE, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_six.id, + entities=[child_entity3.versioning.draft], # specify the version for "pinned" + entities_action=containers_api.ChildrenEntitiesAction.REMOVE, + ) # Now it looks like: assert containers_api.get_entities_in_container(parent_of_six, published=False) == [ @@ -618,6 +612,7 @@ def test_create_next_container_version_with_remove_3( def test_create_next_container_version_with_remove_4( + lp: LearningPackage, parent_of_six: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -636,13 +631,12 @@ def test_create_next_container_version_with_remove_4( Entry(child_entity3.versioning.draft, pinned=False), # entity 3, unpinned ] # Remove "entity 3 unpinned" - should remove only one: - containers_api.create_next_container_version( - parent_of_six.id, - entities=[child_entity3], - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.REMOVE, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_six.id, + entities=[child_entity3], + entities_action=containers_api.ChildrenEntitiesAction.REMOVE, + ) # Now it looks like: assert containers_api.get_entities_in_container(parent_of_six, published=False) == [ @@ -655,7 +649,7 @@ def test_create_next_container_version_with_remove_4( ] -def test_create_next_container_version_with_conflicting_version(parent_of_two: TestContainer): +def test_create_next_container_version_with_conflicting_version(lp: LearningPackage, parent_of_two: TestContainer): """ Test that an appropriate error is raised when calling `create_next_container_version` and specifying a version number that already exists. @@ -663,13 +657,12 @@ def test_create_next_container_version_with_conflicting_version(parent_of_two: T def create_v5(): """Create a new version, specifying version number 5 and changing the title and the order of the children.""" - containers_api.create_next_container_version( - parent_of_two.id, - title="New version - forced as v5", - force_version_num=5, - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_two.id, + title="New version - forced as v5", + force_version_num=5, + ) # First it should work: create_v5() @@ -678,32 +671,30 @@ def create_v5(): create_v5() -def test_create_next_container_version_uninstalled_plugin(container_of_uninstalled_type: Container): +def test_create_next_container_version_uninstalled_plugin(lp: LearningPackage, container_of_uninstalled_type: Container): """ Test that an appropriate error is raised when calling `create_next_container_version` for a container whose type implementation is no longer installed. Such containers should still be readable but not writable. """ with pytest.raises(containers_api.ContainerImplementationMissingError): - containers_api.create_next_container_version( - container_of_uninstalled_type.id, - title="New version of the container", - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + container_of_uninstalled_type.id, + title="New version of the container", + ) -def test_create_next_container_version_other_lp(parent_of_two: TestContainer, other_lp_child: PublishableEntity): +def test_create_next_container_version_other_lp(lp: LearningPackage, parent_of_two: TestContainer, other_lp_child: PublishableEntity): """ Test that an appropriate error is raised when trying to add a child from another learning package to a container. """ with pytest.raises(ValidationError, match="Container entities must be from the same learning package."): - containers_api.create_next_container_version( - parent_of_two.id, - title="Bad Version with entities from another learning package", - created=now, - created_by=None, - entities=[other_lp_child], # <-- from "lp2" Learning Package - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + parent_of_two.id, + title="Bad Version with entities from another learning package", + entities=[other_lp_child], # <-- from "lp2" Learning Package + ) # get_container @@ -730,11 +721,12 @@ def test_get_container_nonexistent() -> None: containers_api.get_container(FAKE_ID) -def test_get_container_soft_deleted(parent_of_two: TestContainer) -> None: +def test_get_container_soft_deleted(lp: LearningPackage, parent_of_two: TestContainer) -> None: """ Test `get_container()` with a soft deleted container """ - publishing_api.soft_delete_draft(parent_of_two.id, deleted_by=None) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.soft_delete_draft(parent_of_two.id) parent_of_two.refresh_from_db() assert parent_of_two.versioning.draft is None assert parent_of_two.versioning.published is None @@ -911,7 +903,8 @@ def test_get_containers_soft_deleted( default, but can be included. """ # Soft delete `parent_of_two`: - publishing_api.soft_delete_draft(parent_of_two.id) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.soft_delete_draft(parent_of_two.id) # Now it should not be included in the result: assert list(containers_api.get_containers(lp.id)) == [ # parent_of_two is not returned. @@ -930,7 +923,7 @@ def test_get_containers_soft_deleted( def test_contains_unpublished_changes_queries( - grandparent: ContainerContainer, child_entity1: TestEntity, django_assert_num_queries + lp: LearningPackage, grandparent: ContainerContainer, child_entity1: TestEntity, django_assert_num_queries ) -> None: """Test that `contains_unpublished_changes()` works, and check how many queries it uses""" # Setup: grandparent and all its descendants are unpublished drafts only. @@ -952,13 +945,12 @@ def test_contains_unpublished_changes_queries( # Now make a tiny change to a grandchild component (not a direct child of "grandparent"), and make sure it's # detected: - publishing_api.create_publishable_entity_version( - child_entity1.pk, - version_num=2, - title="Modified grandchild", - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.create_publishable_entity_version( + child_entity1.pk, + version_num=2, + title="Modified grandchild", + ) child_entity1.refresh_from_db() assert child_entity1.versioning.has_unpublished_changes @@ -1034,19 +1026,18 @@ def test_add_entity_after_publish(lp: LearningPackage, parent_of_two: TestContai assert parent_of_two_v1.version_num == 1 assert parent_of_two.versioning.published is None # Publish everything in the learning package: - publishing_api.publish_all_drafts(lp.id) + publishing_api.publish_all_drafts(lp.id, None) parent_of_two.refresh_from_db() # Reloading is necessary assert not parent_of_two.versioning.has_unpublished_changes # Shallow check assert not containers_api.contains_unpublished_changes(parent_of_two) # Deeper check # Add a published entity (child_entity3, unpinned): - parent_of_two_v2 = containers_api.create_next_container_version( - parent_of_two.id, - entities=[child_entity3], - created=now, - created_by=None, - entities_action=containers_api.ChildrenEntitiesAction.APPEND, - ) + with publishing_api.draft_changes_for(lp.id, None): + parent_of_two_v2 = containers_api.create_next_container_version( + parent_of_two.id, + entities=[child_entity3], + entities_action=containers_api.ChildrenEntitiesAction.APPEND, + ) # Now the container should have unpublished changes: parent_of_two.refresh_from_db() # Reloading the container is necessary assert parent_of_two.versioning.has_unpublished_changes # Shallow check - adding a child changes the container @@ -1131,7 +1122,7 @@ def test_modify_pinned_entity( assert containers_api.get_entities_in_container(parent_of_three, published=False) == expected_contents # Publish everything - publishing_api.publish_all_drafts(lp.id) + publishing_api.publish_all_drafts(lp.id, None) # Now modify the first 📌 pinned child entity (#3) by changing its title (it remains a draft): modify_entity(child_entity3) @@ -1169,25 +1160,22 @@ def test_publishing_shared_component(lp: LearningPackage): c3_v1 = c3.versioning.draft c4_v1 = c4.versioning.draft c5_v1 = c5.versioning.draft - unit1, _ = containers_api.create_container_and_version( - lp.id, - entities=[c1, c2, c3], - title="Unit 1", - container_code="unit-1", - created=now, - created_by=None, - container_cls=TestContainer, - ) - unit2, _ = containers_api.create_container_and_version( - lp.id, - entities=[c2, c4, c5], - title="Unit 2", - container_code="unit-2", - created=now, - created_by=None, - container_cls=TestContainer, - ) - publishing_api.publish_all_drafts(lp.id) + with publishing_api.draft_changes_for(lp.id, None): + unit1, _ = containers_api.create_container_and_version( + lp.id, + entities=[c1, c2, c3], + title="Unit 1", + container_code="unit-1", + container_cls=TestContainer, + ) + unit2, _ = containers_api.create_container_and_version( + lp.id, + entities=[c2, c4, c5], + title="Unit 2", + container_code="unit-2", + container_cls=TestContainer, + ) + publishing_api.publish_all_drafts(lp.id, None) assert containers_api.contains_unpublished_changes(unit1.id) is False assert containers_api.contains_unpublished_changes(unit2.id) is False @@ -1351,6 +1339,7 @@ def test_get_entities_in_container( def test_get_entities_in_container_soft_deletion_unpinned( + lp: LearningPackage, parent_of_three: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -1367,7 +1356,8 @@ def test_get_entities_in_container_soft_deletion_unpinned( # First, publish everything: publish_entity(parent_of_three) # Soft delete the third, unpinned child (child_entity1): - publishing_api.soft_delete_draft(child_entity1.pk) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.soft_delete_draft(child_entity1.pk) # That deletion should NOT count as a change to the container itself: parent_of_three.refresh_from_db() @@ -1384,6 +1374,7 @@ def test_get_entities_in_container_soft_deletion_unpinned( def test_get_entities_in_container_soft_deletion_pinned( + lp: LearningPackage, parent_of_three: TestContainer, child_entity1: TestEntity, child_entity2: TestEntity, @@ -1400,7 +1391,8 @@ def test_get_entities_in_container_soft_deletion_pinned( # First, publish everything: publish_entity(parent_of_three) # Soft delete child 2: - publishing_api.soft_delete_draft(child_entity2.pk) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.soft_delete_draft(child_entity2.pk) # The above deletions should NOT count as a change to the container itself, in any way: parent_of_three.refresh_from_db() @@ -1426,41 +1418,39 @@ def test_snapshots_of_published_unit(lp: LearningPackage, child_entity1: TestEnt assert not before_publish # Empty list # Publish everything, creating Checkpoint 1 - checkpoint_1 = publishing_api.publish_all_drafts(lp.id, message="checkpoint 1") + checkpoint_1 = publishing_api.publish_all_drafts(lp.id, None, message="checkpoint 1") ######################################################################## # Now we update the title of the component. modify_entity(child_entity1, title="Component 1 as of checkpoint 2") # Publish everything, creating Checkpoint 2 - checkpoint_2 = publishing_api.publish_all_drafts(lp.id, message="checkpoint 2") + checkpoint_2 = publishing_api.publish_all_drafts(lp.id, None, message="checkpoint 2") ######################################################################## # Now add a second component to the unit: modify_entity(child_entity1, title="Component 1 as of checkpoint 3") modify_entity(child_entity2, title="Component 2 as of checkpoint 3") - containers_api.create_next_container_version( - container.id, - title="Unit title in checkpoint 3", - entities=[child_entity1, child_entity2], - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + container.id, + title="Unit title in checkpoint 3", + entities=[child_entity1, child_entity2], + ) # Publish everything, creating Checkpoint 3 - checkpoint_3 = publishing_api.publish_all_drafts(lp.id, message="checkpoint 3") + checkpoint_3 = publishing_api.publish_all_drafts(lp.id, None, message="checkpoint 3") ######################################################################## # Now add a third component to the unit, a pinned 📌 version of component 1. # This will test pinned versions and also test adding at the beginning rather than the end of the unit. - containers_api.create_next_container_version( - container.id, - title="Unit title in checkpoint 4", - entities=[child_entity1_v1, child_entity1, child_entity2], - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + container.id, + title="Unit title in checkpoint 4", + entities=[child_entity1_v1, child_entity1, child_entity2], + ) # Publish everything, creating Checkpoint 4 - checkpoint_4 = publishing_api.publish_all_drafts(lp.id, message="checkpoint 4") + checkpoint_4 = publishing_api.publish_all_drafts(lp.id, None, message="checkpoint 4") ######################################################################## # Modify the drafts, but don't publish: @@ -1560,7 +1550,7 @@ def test_get_container_children_count( grandparent: ContainerContainer, ): """Test `get_container_children_count()`""" - publishing_api.publish_all_drafts(lp.id) + publishing_api.publish_all_drafts(lp.id, None) assert containers_api.get_container_children_count(parent_of_two, published=False) == 2 assert containers_api.get_container_children_count(parent_of_two, published=True) == 2 @@ -1574,12 +1564,11 @@ def test_get_container_children_count( assert containers_api.get_container_children_count(grandparent, published=True) == 2 # Add another container to "grandparent": - containers_api.create_next_container_version( - grandparent, - entities=[parent_of_two, parent_of_three, parent_of_six], - created=now, - created_by=None, - ) + with publishing_api.draft_changes_for(lp.id, None): + containers_api.create_next_container_version( + grandparent, + entities=[parent_of_two, parent_of_three, parent_of_six], + ) # Warning: this is required if 'grandparent' is passed by ID to `create_next_container_version()`: # grandparent.refresh_from_db() assert containers_api.get_container_children_count(grandparent, published=False) == 3 @@ -1593,8 +1582,9 @@ def test_get_container_children_count_soft_deletion( child_entity2: TestEntity, ): """Test `get_container_children_count()` when an entity is soft deleted""" - publishing_api.publish_all_drafts(lp.id) - publishing_api.soft_delete_draft(child_entity2.pk) + publishing_api.publish_all_drafts(lp.id, None) + with publishing_api.draft_changes_for(lp.id, None): + publishing_api.soft_delete_draft(child_entity2.pk) # "parent_of_two" contains the soft deleted child, so its draft child count is decreased by one: assert containers_api.get_container_children_count(parent_of_two, published=False) == 1 assert containers_api.get_container_children_count(parent_of_two, published=True) == 2 @@ -1611,7 +1601,7 @@ def test_get_container_children_count_queries( django_assert_num_queries, ): """Test how many database queries `get_container_children_count()` needs""" - publishing_api.publish_all_drafts(lp.id) + publishing_api.publish_all_drafts(lp.id, None) # The 6 queries are: # - Draft.objects.get() # - PublishableEntityVersion.objects.get() diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index c751aeef7..5be736387 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -2,12 +2,13 @@ Basic tests for the sections API. """ -from typing import Any, cast +from typing import cast import pytest from django.core.exceptions import ValidationError import openedx_content.api as content_api +from openedx_content.applets.publishing import api as publishing_api from openedx_content.models_api import Section, SectionVersion, Subsection, SubsectionVersion from ..components.test_api import ComponentTestCase @@ -29,35 +30,31 @@ def setUp(self) -> None: component_code="component_2", title="Great-grandchild component", ) - common_args: dict[str, Any] = { - "learning_package_id": self.learning_package.id, - "created": self.now, - "created_by": None, - } - self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( - container_code="unit_1", - title="Grandchild Unit 1", - components=[self.component_1, self.component_2], - **common_args, - ) - self.unit_2, self.unit_2_v1 = content_api.create_unit_and_version( - container_code="unit_2", - title="Grandchild Unit 2", - components=[self.component_2, self.component_1], # Backwards order from Unit 1 - **common_args, - ) - self.subsection_1, self.subsection_1_v1 = content_api.create_subsection_and_version( - container_code="subsection_1", - title="Child Subsection 1", - units=[self.unit_1, self.unit_2], - **common_args, - ) - self.subsection_2, self.subsection_2_v1 = content_api.create_subsection_and_version( - container_code="subsection_2", - title="Child Subsection 2", - units=[self.unit_2, self.unit_1], # Backwards order from subsection 1 - **common_args, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( + learning_package_id=self.learning_package.id, + container_code="unit_1", + title="Grandchild Unit 1", + components=[self.component_1, self.component_2], + ) + self.unit_2, self.unit_2_v1 = content_api.create_unit_and_version( + learning_package_id=self.learning_package.id, + container_code="unit_2", + title="Grandchild Unit 2", + components=[self.component_2, self.component_1], # Backwards order from Unit 1 + ) + self.subsection_1, self.subsection_1_v1 = content_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + container_code="subsection_1", + title="Child Subsection 1", + units=[self.unit_1, self.unit_2], + ) + self.subsection_2, self.subsection_2_v1 = content_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + container_code="subsection_2", + title="Child Subsection 2", + units=[self.unit_2, self.unit_1], # Backwards order from subsection 1 + ) def create_section_with_subsections( self, @@ -67,14 +64,13 @@ def create_section_with_subsections( container_code="section-key", ) -> Section: """Helper method to quickly create a section with some subsections""" - section, _section_v1 = content_api.create_section_and_version( - learning_package_id=self.learning_package.id, - container_code=container_code, - title=title, - subsections=subsections, - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + section, _section_v1 = content_api.create_section_and_version( + learning_package_id=self.learning_package.id, + container_code=container_code, + title=title, + subsections=subsections, + ) return section def test_create_empty_section_and_version(self) -> None: @@ -86,13 +82,12 @@ def test_create_empty_section_and_version(self) -> None: 3. The section is a draft with unpublished changes. 4. There is no published version of the section. """ - section, section_version = content_api.create_section_and_version( - learning_package_id=self.learning_package.id, - container_code="section-key", - title="Section", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + section, section_version = content_api.create_section_and_version( + learning_package_id=self.learning_package.id, + container_code="section-key", + title="Section", + ) assert isinstance(section, Section) assert isinstance(section_version, SectionVersion) assert section, section_version @@ -113,13 +108,12 @@ def test_create_next_section_version_with_unpinned_subsections(self) -> None: 4. The unit is in the draft section version's subsection list and is unpinned. """ section = self.create_section_with_subsections([]) - section_version_v2 = content_api.create_next_section_version( - section, - title="Section", - subsections=[self.subsection_1], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + section_version_v2 = content_api.create_next_section_version( + section, + title="Section", + subsections=[self.subsection_1], + ) assert isinstance(section_version_v2, SectionVersion) assert section_version_v2.version_num == 2 assert section_version_v2 in section.versioning.versions.all() @@ -159,6 +153,7 @@ def test_section_queries(self) -> None: content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=section.id), + published_by=None, ) with self.assertNumQueries(4): result = content_api.get_subsections_in_section(section, published=True) @@ -179,12 +174,11 @@ def test_create_section_with_invalid_children(self): ValidationError, match='The entity "unit_1" cannot be added to a "section" container.', ): - content_api.create_next_section_version( - section, - subsections=[self.unit_1], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + content_api.create_next_section_version( + section, + subsections=[self.unit_1], + ) # Check that a new version was not created: section.refresh_from_db() assert content_api.get_section(section.id).versioning.draft == section_version diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index 55b833b87..6b8a05aae 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -8,6 +8,7 @@ from django.core.exceptions import ValidationError import openedx_content.api as content_api +from openedx_content.applets.publishing import api as publishing_api from openedx_content.models_api import Subsection, SubsectionVersion, Unit, UnitVersion from ..components.test_api import ComponentTestCase @@ -28,14 +29,13 @@ def setUp(self) -> None: component_code="Query_Counting_2", title="Querying Counting Problem (2)", ) - self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( - learning_package_id=self.learning_package.id, - container_code="unit1", - title="Unit 1", - components=[self.component_1, self.component_2], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( + learning_package_id=self.learning_package.id, + container_code="unit1", + title="Unit 1", + components=[self.component_1, self.component_2], + ) def create_subsection_with_units( self, @@ -45,14 +45,13 @@ def create_subsection_with_units( container_code="subsection-key", ) -> Subsection: """Helper method to quickly create a unit with some units""" - subsection, _subsection_v1 = content_api.create_subsection_and_version( - learning_package_id=self.learning_package.id, - container_code=container_code, - title=title, - units=units, - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + subsection, _subsection_v1 = content_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + container_code=container_code, + title=title, + units=units, + ) return subsection def test_create_empty_subsection_and_version(self): @@ -64,13 +63,12 @@ def test_create_empty_subsection_and_version(self): 3. The subsection is a draft with unpublished changes. 4. There is no published version of the subsection. """ - subsection, subsection_version = content_api.create_subsection_and_version( - learning_package_id=self.learning_package.id, - container_code="subsection-key", - title="Subsection", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + subsection, subsection_version = content_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + container_code="subsection-key", + title="Subsection", + ) assert isinstance(subsection, Subsection) assert isinstance(subsection_version, SubsectionVersion) assert subsection, subsection_version @@ -91,13 +89,12 @@ def test_create_next_subsection_version_with_unpinned_unit(self): 4. The unit is in the draft subsection version's unit list and is unpinned. """ subsection = self.create_subsection_with_units([]) - subsection_version_v2 = content_api.create_next_subsection_version( - subsection, - title="Subsection", - units=[self.unit_1], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + subsection_version_v2 = content_api.create_next_subsection_version( + subsection, + title="Subsection", + units=[self.unit_1], + ) assert isinstance(subsection_version_v2, SubsectionVersion) assert subsection_version_v2.version_num == 2 assert subsection_version_v2 in subsection.versioning.versions.all() @@ -137,6 +134,7 @@ def test_subsection_queries(self) -> None: content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=subsection.id), + published_by=None, ) with self.assertNumQueries(4): result = content_api.get_units_in_subsection(subsection, published=True) @@ -158,12 +156,11 @@ def test_create_subsection_with_invalid_children(self): ValidationError, match='The entity "xblock.v1:problem:Query_Counting" cannot be added to a "subsection" container.', ) as err: - content_api.create_next_subsection_version( - subsection, - units=[self.component_1], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + content_api.create_next_subsection_version( + subsection, + units=[self.component_1], + ) assert "(found non-Container child)" in str(err.value.__cause__) # Check that a new version was not created: diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index 36e27155f..1db60f8f3 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -8,6 +8,7 @@ from django.core.exceptions import ValidationError import openedx_content.api as content_api +from openedx_content.applets.publishing import api as publishing_api from openedx_content.models_api import Component, ComponentVersion, Unit, UnitVersion from tests.test_django_app.models import TestContainer @@ -38,14 +39,13 @@ def create_unit_with_components( container_code="unit-key", ) -> Unit: """Helper method to quickly create a unit with some components""" - unit, _unit_v1 = content_api.create_unit_and_version( - learning_package_id=self.learning_package.id, - container_code=container_code, - title=title, - components=components, - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + unit, _unit_v1 = content_api.create_unit_and_version( + learning_package_id=self.learning_package.id, + container_code=container_code, + title=title, + components=components, + ) return unit def test_create_empty_unit_and_version(self): @@ -57,13 +57,12 @@ def test_create_empty_unit_and_version(self): 3. The unit is a draft with unpublished changes. 4. There is no published version of the unit. """ - unit, unit_version = content_api.create_unit_and_version( - learning_package_id=self.learning_package.id, - container_code="unit-key", - title="Unit", - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + unit, unit_version = content_api.create_unit_and_version( + learning_package_id=self.learning_package.id, + container_code="unit-key", + title="Unit", + ) assert isinstance(unit, Unit) assert isinstance(unit_version, UnitVersion) assert unit, unit_version @@ -84,13 +83,12 @@ def test_create_next_unit_version_with_two_unpinned_components(self): 4. The components are in the draft unit version's component list and are unpinned. """ unit = self.create_unit_with_components([]) - unit_version_v2 = content_api.create_next_unit_version( - unit, - title="Unit", - components=[self.component_1, self.component_2], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + unit_version_v2 = content_api.create_next_unit_version( + unit, + title="Unit", + components=[self.component_1, self.component_2], + ) assert isinstance(unit_version_v2, UnitVersion) assert unit_version_v2.version_num == 2 assert unit_version_v2 in unit.versioning.versions.all() @@ -138,6 +136,7 @@ def test_unit_queries(self) -> None: content_api.publish_from_drafts( self.learning_package.id, draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=unit.id), + published_by=None, ) with self.assertNumQueries(3): result = content_api.get_components_in_unit(unit, published=True) @@ -159,26 +158,25 @@ def test_create_unit_with_invalid_children(self): with pytest.raises( ValidationError, match='The entity "unit-key2" cannot be added to a "unit" container.' ) as err: - content_api.create_next_unit_version( - unit, - components=[unit2], - created=self.now, - created_by=None, - ) + with publishing_api.draft_changes_for(self.learning_package.id, None): + content_api.create_next_unit_version( + unit, + components=[unit2], + ) assert "Only Components can be added as children of a Unit" in str(err.value.__cause__) # Try adding a generic entity to a Unit - pe = content_api.create_publishable_entity(self.learning_package.id, "pe", created=self.now, created_by=None) - pev = content_api.create_publishable_entity_version( - pe.id, version_num=1, title="t", created=self.now, created_by=None - ) - with pytest.raises(ValidationError, match='The entity "pe" cannot be added to a "unit" container.') as err: - content_api.create_next_unit_version( - unit, - components=[pev], - created=self.now, - created_by=None, + with publishing_api.draft_changes_for(self.learning_package.id, None): + pe = content_api.create_publishable_entity(self.learning_package.id, "pe") + pev = content_api.create_publishable_entity_version( + pe.id, version_num=1, title="t", ) + with pytest.raises(ValidationError, match='The entity "pe" cannot be added to a "unit" container.') as err: + with publishing_api.draft_changes_for(self.learning_package.id, None): + content_api.create_next_unit_version( + unit, + components=[pev], + ) assert "Only Components can be added as children of a Unit" in str(err.value.__cause__) # Check that a new version was not created: