From c90c97a28e566cbf2eca985a4247467fb9fc8870 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 27 Feb 2025 16:33:47 -0500 Subject: [PATCH 01/29] feat: grouped draft changes (DraftChangeLog) Create a way to group Draft modifications, as well as represent side-effects (for the parent-child use case). (Need to flesh this out a lot more before we merge.) --- .../apps/authoring/publishing/admin.py | 75 ++++- .../apps/authoring/publishing/api.py | 292 ++++++++++++++++-- .../authoring/publishing/contextmanagers.py | 120 +++++++ .../migrations/0004_draftchangelog.py | 66 ++++ .../authoring/publishing/models/__init__.py | 4 +- .../authoring/publishing/models/draft_log.py | 199 ++++++++++++ .../publishing/models/draft_published.py | 95 ------ .../publishing/models/publish_log.py | 44 +++ .../apps/authoring/publishing/test_api.py | 187 +++++++++-- .../apps/authoring/units/test_api.py | 4 +- 10 files changed, 938 insertions(+), 148 deletions(-) create mode 100644 openedx_learning/apps/authoring/publishing/contextmanagers.py create mode 100644 openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py create mode 100644 openedx_learning/apps/authoring/publishing/models/draft_log.py delete mode 100644 openedx_learning/apps/authoring/publishing/models/draft_published.py diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index c36262b3e..b8b351286 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -4,11 +4,19 @@ from __future__ import annotations from django.contrib import admin +from django.db.models import Count +from .models.publish_log import Published from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin, one_to_one_related_model_html -from .models import LearningPackage, PublishableEntity, Published, PublishLog, PublishLogRecord - +from .models import ( + DraftChangeLogRecord, + DraftChangeLog, + LearningPackage, + PublishableEntity, + PublishLog, + PublishLogRecord, +) @admin.register(LearningPackage) class LearningPackageAdmin(ReadOnlyModelAdmin): @@ -171,3 +179,66 @@ def published_at(self, published_obj): def message(self, published_obj): return published_obj.publish_log_record.publish_log.message + + +class DraftChangeTabularInline(admin.TabularInline): + model = DraftChangeLogRecord + + fields = ( + "entity", + "title", + "old_version_num", + "new_version_num", + ) + readonly_fields = fields + + def get_queryset(self, request): + queryset = super().get_queryset(request) + return queryset.select_related("entity", "old_version", "new_version") \ + .order_by("entity__key") + + def old_version_num(self, draft_change: DraftChangeLogRecord): + if draft_change.old_version is None: + return "-" + return draft_change.old_version.version_num + + def new_version_num(self, draft_change: DraftChangeLogRecord): + if draft_change.new_version is None: + return "-" + return draft_change.new_version.version_num + + def title(self, draft_change: DraftChangeLogRecord): + """ + Get the title to display for the DraftChange + """ + if draft_change.new_version: + return draft_change.new_version.title + if draft_change.old_version: + return draft_change.old_version.title + return "" + + +@admin.register(DraftChangeLog) +class DraftChangeSetAdmin(ReadOnlyModelAdmin): + """ + Read-only admin to view Draft changes (via inline tables) + """ + inlines = [DraftChangeTabularInline] + fields = ( + "uuid", + "learning_package", + "num_changes", + "changed_at", + "changed_by", + ) + readonly_fields = fields + list_display = fields + list_filter = ["learning_package",] + + def num_changes(self, draft_change_set): + return draft_change_set.num_changes + + def get_queryset(self, request): + queryset = super().get_queryset(request) + return queryset.select_related("learning_package", "changed_by") \ + .annotate(num_changes=Count("changes")) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 52f666ac3..c6198bec7 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -6,19 +6,27 @@ """ from __future__ import annotations +from contextlib import nullcontext from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum +from functools import singledispatch from typing import TypeVar from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet from django.db.transaction import atomic +from .models.publish_log import Published + +from .contextmanagers import DraftChangeLogContext from .models import ( Container, ContainerVersion, Draft, + DraftChangeLogRecord, + DraftChangeLog, + DraftSideEffect, EntityList, EntityListRow, LearningPackage, @@ -27,7 +35,6 @@ PublishableEntityMixin, PublishableEntityVersion, PublishableEntityVersionMixin, - Published, PublishLog, PublishLogRecord, ) @@ -82,6 +89,7 @@ "contains_unpublished_changes", "get_containers_with_entity", "get_container_children_count", + "bulk_draft_changes_for", ] @@ -220,10 +228,14 @@ def create_publishable_entity_version( created=created, created_by_id=created_by, ) - Draft.objects.update_or_create( - entity_id=entity_id, - defaults={"version": version}, + set_draft_version( + entity_id, + version.id, + set_at=created, + set_by=created_by, + create_transaction=False, ) + return version @@ -446,10 +458,14 @@ def get_published_version(publishable_entity_id: int, /) -> PublishableEntityVer return published.version +@singledispatch def set_draft_version( - publishable_entity_id: int, + draft: Draft, publishable_entity_version_pk: int | None, /, + set_at: datetime | None = None, + set_by: int | None = None, # User.id + create_transaction: bool = True, ) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. @@ -459,13 +475,199 @@ def set_draft_version( edited some content). Setting a Draft's version to None is like deleting it from Studio's editing point of view (see ``soft_delete_draft`` for more details). + + Calling this function attaches a new DraftChange 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 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) + + tx_context = atomic() if create_transaction else nullcontext() + + with tx_context: + old_version_id = draft.version_id + draft.version_id = publishable_entity_version_pk + draft.save() + + learning_package_id = draft.entity.learning_package_id + + # 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. + active_change_log = DraftChangeLogContext.get_active_draft_change_log(learning_package_id) + change_log = active_change_log or DraftChangeLog.objects.create( + learning_package_id=learning_package_id, + changed_at=set_at, + changed_by_id=set_by, + ) + + if active_change_log: + change = _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, + ) + # We explicitly *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 the child Component. So we'll let DraftChangeLogContext + # do that work when it exits its context. + else: + # This means there is no active DraftChangeLog, so we create our own + # and add our DraftChangeLogRecord to it. This has the minor + # optimization that we don't have to check for an existing + # DraftChangeLogRecord, because we know it can't exist yet. + change = 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, + ) + _create_container_side_effects_for_draft_change(change) + + +@set_draft_version.register(int) +def _( + publishable_entity_id: int, + publishable_entity_version_pk: int | None, + /, + set_at: datetime | None = None, + set_by: int | None = None, # User.id + create_transaction: bool = True, +) -> None: + """ + Alias for set_draft_version taking PublishableEntity.id instead of a Draft. + """ + tx_context = atomic() if create_transaction else nullcontext() + with tx_context: + draft, _created = Draft.objects.select_related("entity") \ + .get_or_create(entity_id=publishable_entity_id) + set_draft_version( + draft, + publishable_entity_version_pk, + set_at=set_at, + set_by=set_by, + create_transaction=False, + ) + + +def _add_to_existing_draft_change_log( + active_change_log: DraftChangeLog, + entity_id: int, + old_version_id: int | None, + new_version_id: int | None, +) -> DraftChangeLogRecord: + # If we get here, it means there is an active DraftChangeLog that may + # have many DraftChanges associated with it. A DraftChangeLog can only + # have one DraftChange per PublishableEntity, e.g. the same Component + # can't go from v1 to v3 and v1 to v4 in the same DraftChangeLog. + try: + # If there was already a DraftChange for this PublishableEntity, + # we update the new_version_id. We keep the old_version_id value + # though, because that represents what it was before this + # DraftChangeLog, and we don't want to lose that information. + change = DraftChangeLogRecord.objects.get( + draft_change_log=active_change_log, + entity_id=entity_id, + ) + change.new_version_id = new_version_id + change.save() + except DraftChangeLogRecord.DoesNotExist: + # If we're here, this is the first DraftChange we're making for + # this PublishableEntity in the active DraftChangeLog. + change = DraftChangeLogRecord.objects.create( + draft_change_log=active_change_log, + entity_id=entity_id, + old_version_id=old_version_id, + new_version_id=new_version_id, + ) + + return change + + +def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLogRecord): + processed_entity_ids = set() + for change in change_log.changes.all(): + _create_container_side_effects_for_draft_change( + change, + processed_entity_ids=processed_entity_ids, + ) + + +def _create_container_side_effects_for_draft_change( + original_change: DraftChangeLogRecord, + processed_entity_ids: set | None = None +): """ - draft = Draft.objects.get(entity_id=publishable_entity_id) - draft.version_id = publishable_entity_version_pk - draft.save() + Given a draft change, add side effects for all affected containers. + + This should only be run after the DraftChangeLogRecord has been otherwise + fully written out. We want to avoid the scenario where we create a + side-effect that a Component change affects a Unit if the Unit version is + also changed in the same DraftChangeLog. + + TODO: This could get very expensive with the get_containers_with_entity + calls. We should measure the impact of this. + """ + if processed_entity_ids is None: + processed_entity_ids = set() + + changes_and_containers = [ + (original_change, container) + for container + in get_containers_with_entity(original_change.entity_id, ignore_pinned=True) + ] + while changes_and_containers: + change, container = changes_and_containers.pop() + + # If the container is not already in the DraftChangeLog, we need to + # add it. Since it's being caused as a DraftSideEffect, we're going + # add it with the old_version == new_version convention. + container_draft_version_pk = container.versioning.draft.pk + container_change, _created = DraftChangeLogRecord.objects.get_or_create( + draft_change_log=change.draft_change_log, + entity_id=container.pk, + old_version_id=container_draft_version_pk, + defaults={ + 'new_version_id': container_draft_version_pk + } + ) + + # Mark that change in the current loop has the side effect of changing + # the parent container. We'll do this regardless of whether the + # container version itself also changed. If a Unit has a Component and + # both the Unit and Component have their versions incremented, then the + # Unit has changed in both ways (the Unit's internal metadata as well as + # the new version of the child component). + DraftSideEffect.objects.get_or_create(cause=change, effect=container_change) + processed_entity_ids.add(change.entity_id) + + # Now we find the next layer up of containers. So if the originally + # passed in publishable_entity_id was for a Component, then the + # ``container`` we've been creating the side effect for in this loop + # is the Unit, and ``parents_of_container`` would be any Sequences + # that contain the Unit. + parents_of_container = get_containers_with_entity(container.pk, ignore_pinned=True) + + changes_and_containers.extend( + (container_change, container_parent) + for container_parent in parents_of_container + if container_parent.pk not in processed_entity_ids + ) -def soft_delete_draft(publishable_entity_id: int, /) -> None: +def soft_delete_draft(publishable_entity_id: int, /, deleted_by: int | None = None) -> None: """ Sets the Draft version to None. @@ -475,10 +677,15 @@ def soft_delete_draft(publishable_entity_id: int, /) -> None: of pointing the Draft back to the most recent ``PublishableEntityVersion`` for a given ``PublishableEntity``. """ - return set_draft_version(publishable_entity_id, None) + return set_draft_version(publishable_entity_id, None, set_by=deleted_by) -def reset_drafts_to_published(learning_package_id: int, /) -> None: +def reset_drafts_to_published( + learning_package_id: int, + /, + reset_at: datetime | None = None, + reset_by: int | None = None, # User.id +) -> None: """ Reset all Drafts to point to the most recently Published versions. @@ -506,23 +713,53 @@ def reset_drafts_to_published(learning_package_id: int, /) -> None: Also, there is no current immutable record for when a reset happens. It's not like a publish that leaves an entry in the ``PublishLog``. """ + if reset_at is None: + reset_at = datetime.now(tz=timezone.utc) + # These are all the drafts that are different from the published versions. draft_qset = Draft.objects \ .select_related("entity__published") \ .filter(entity__learning_package_id=learning_package_id) \ - .exclude(entity__published__version_id=F("version_id")) + .exclude(entity__published__version_id=F("version_id")) \ + .exclude( + # NULL != NULL in SQL, so we want to exclude entries + # where both the published version and draft version + # are None. This edge case happens when we create + # something and then delete it without publishing, and + # then reset Drafts to their published state. + Q(entity__published__version__isnull=True) & + Q(version__isnull=True) + ) + # If there's nothing to reset because there are no changes from the + # published version, just return early rather than making an empty + # DraftChangeLog. + 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. + 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 + ) - # 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. We can - # rework this into a bulk update or custom SQL if it becomes a performance - # issue. - with atomic(): + 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'): - draft.version_id = draft.entity.published.version_id + published_version_id = draft.entity.published.version_id else: - draft.version = None - draft.save() + published_version_id = None + + set_draft_version(draft, published_version_id, create_transaction=False) def register_content_models( @@ -1103,6 +1340,7 @@ def get_containers_with_entity( ignore_pinned: if true, ignore any pinned references to the entity. """ if ignore_pinned: + # TODO: Do we need to run distinct() on this? qs = Container.objects.filter( # Note: these two conditions must be in the same filter() call, or the query won't be correct. publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 @@ -1145,3 +1383,17 @@ def get_container_children_count( else: filter_deleted = {"entity__draft__version__isnull": False} return container_version.entity_list.entitylistrow_set.filter(**filter_deleted).count() + + +def bulk_draft_changes_for(learning_package_id: int, changed_by=None, changed_at=None) -> DraftChangeLog: + """ + Context manager to do a single batch of Draft changes in. + """ + return DraftChangeLogContext( + learning_package_id, + changed_at=changed_at, + changed_by=changed_by, + exit_callbacks=[ + _create_container_side_effects_for_draft_change_log, + ] + ) diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py new file mode 100644 index 000000000..32e628bf0 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -0,0 +1,120 @@ +from contextvars import ContextVar +from datetime import datetime, timezone + +from django.db.transaction import Atomic + +from .models import DraftChangeLog + + +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 + manager directly, since this is a bit experimental and the implementation + may shift around a bit. + + The main idea is that we want to create a context manager that will keep + track of what the "active" DraftChangeChangeLogs is for a given + LearningPackage. The problem is that declaring the context can happen many + layers higher in the stack than doing the draft changes. For instance, + imagine:: + + with bulk_draft_changes_for(learning_package.id): + for section in course: + update_section_drafts(learning_package_id, section) + + In this hypothetical code block, update_section_drafts might call a function + to update sequences, which calls something to update units, etc. Only at the + very bottom layer of the stack will those code paths actually alter the + drafts themselves. It would make the API too cumbersome to explicitly pass + the active DraftChangeChangeLog through every layer. So instead, we use this + class to essentially keep the active DraftChangeChangeLog in a global + (ContextVar) so that the publishing API draft-related code can access it + later. + + Since it is possible to nest context managers, we keep a list of the + DraftChangeChangeLogs that have been created and treat it like a stack that + gets pushed to whenever we __enter__ and popped off whenever we __exit__. + + DraftChangeLogContext also subclasses Django's Atomic context manager, since + any operation on multiple Drafts as part of a DraftChangeLog will want to be + an atomic operation. + """ + _draft_change_logs: ContextVar[list | None] = ContextVar('_draft_change_logs', default=None) + + def __init__(self, learning_package_id, changed_at=None, changed_by=None, exit_callbacks=None): + super().__init__(using=None, savepoint=False, durable=False) + + self.learning_package_id = learning_package_id + self.changed_by = changed_by + self.changed_at = changed_at or datetime.now(tz=timezone.utc) + + # We currently use self.exit_callbacks as a way to ruh parent/child + # side-effect creation. DraftChangeLogContext itself is a lower-level + # part of the code that doesn't understand what containers are. + self.exit_callbacks = exit_callbacks or [] + + @classmethod + def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog | None: + """ + Get the DraftChangeLogContext for new DraftChangeLogRecords. + + This is expected to be called internally by the publishing API when it + modifies Drafts. If there is no active DraftChangeLog, this method will + return None, and the caller should create their own DraftChangeLog. + """ + draft_change_logs = cls._draft_change_logs.get() + + # If we've never used this manager... + if draft_change_logs is None: + return None + + # Otherwise, find the most recently created DraftChangeLog *that matches + # the learning_package_id*. This is for two reasons: + # + # 1. We might nest operations so that we're working across multiple + # LearningPackages at once, e.g. copying content from different + # libraries and importing them into another library. + # 2. It's a guard in case we somehow got the global state management + # wrong. We want the failure mode to be "we didn't find the + # DraftChangeLog you were looking for, so make another one and suffer + # a performance penalty", as opposed to, "we accidentally gave you a + # DraftChangeLog for an entirely different LearningPackage, and now + # your Draft data is corrupted." + for draft_change_log in reversed(draft_change_logs): + if draft_change_log.learning_package_id == learning_package_id: + return draft_change_log + + # If we got here, then either the list was empty (the manager was used + # at some point but exited), or none of the DraftChangeLogs are for the + # correct LearningPackage. + return None + + def __enter__(self): + super().__enter__() + + self.draft_change_log = DraftChangeLog.objects.create( + learning_package_id=self.learning_package_id, + changed_by=self.changed_by, + changed_at=self.changed_at, + ) + draft_change_sets = self._draft_change_logs.get() + if not draft_change_sets: + draft_change_sets = [] + draft_change_sets.append(self.draft_change_log) + self._draft_change_logs.set(draft_change_sets) + + return self.draft_change_log + + def __exit__(self, type, value, traceback): + draft_change_logs = self._draft_change_logs.get() + if draft_change_logs: + draft_change_log = draft_change_logs.pop() + for exit_callback in self.exit_callbacks: + exit_callback(draft_change_log) + + self._draft_change_logs.set(draft_change_logs) + + return super().__exit__(type, value, traceback) diff --git a/openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py new file mode 100644 index 000000000..1ad9d905d --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py @@ -0,0 +1,66 @@ +# Generated by Django 4.2.18 on 2025-03-28 02:21 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import openedx_learning.lib.validators +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('oel_publishing', '0003_containers'), + ] + + operations = [ + migrations.CreateModel( + name='DraftChangeLog', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('changed_at', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), + ('changed_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), + ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), + ], + options={ + 'verbose_name': 'Draft Change Log', + 'verbose_name_plural': 'Draft Change Logs', + }, + ), + migrations.CreateModel( + name='DraftChangeLogRecord', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('draft_change_log', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='changes', to='oel_publishing.draftchangelog')), + ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), + ('new_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentityversion')), + ('old_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')), + ], + options={ + 'verbose_name': 'Draft Log', + 'verbose_name_plural': 'Draft Log', + }, + ), + migrations.CreateModel( + name='DraftSideEffect', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('cause', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='causes', to='oel_publishing.draftchangelogrecord')), + ('effect', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='caused_by', to='oel_publishing.draftchangelogrecord')), + ], + ), + migrations.AddConstraint( + model_name='draftsideeffect', + constraint=models.UniqueConstraint(fields=('cause', 'effect'), name='oel_pub_dse_uniq_c_e'), + ), + migrations.AddIndex( + model_name='draftchangelogrecord', + index=models.Index(fields=['entity', '-draft_change_log'], name='oel_dlr_idx_entity_rdcl'), + ), + migrations.AddConstraint( + model_name='draftchangelogrecord', + constraint=models.UniqueConstraint(fields=('draft_change_log', 'entity'), name='oel_dlr_uniq_dcl'), + ), + ] diff --git a/openedx_learning/apps/authoring/publishing/models/__init__.py b/openedx_learning/apps/authoring/publishing/models/__init__.py index 27329076d..324cf27d3 100644 --- a/openedx_learning/apps/authoring/publishing/models/__init__.py +++ b/openedx_learning/apps/authoring/publishing/models/__init__.py @@ -14,10 +14,10 @@ """ from .container import Container, ContainerVersion -from .draft_published import Draft, Published +from .draft_log import Draft, DraftChangeLogRecord, DraftChangeLog, DraftSideEffect from .entity_list import EntityList, EntityListRow from .learning_package import LearningPackage -from .publish_log import PublishLog, PublishLogRecord +from .publish_log import PublishLog, PublishLogRecord, Published from .publishable_entity import ( PublishableContentModelRegistry, PublishableEntity, diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py new file mode 100644 index 000000000..ab1a6ffd7 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -0,0 +1,199 @@ +""" +Models relating to the creation and management of Draft information. +""" +from django.conf import settings +from django.db import models +from django.utils.translation import gettext_lazy as _ + +from openedx_learning.lib.fields import immutable_uuid_field, manual_date_time_field + +from .learning_package import LearningPackage +from .publishable_entity import PublishableEntity, PublishableEntityVersion + + +class Draft(models.Model): + """ + Find the active draft version of an entity (usually most recently created). + + This model mostly only exists to allow us to join against a bunch of + PublishableEntity objects at once and get all their latest drafts. You might + use this together with Published in order to see which Drafts haven't been + published yet. + + A Draft entry should be created whenever a new PublishableEntityVersion is + created. This means there are three possible states: + + 1. No Draft entry for a PublishableEntity: This means a PublishableEntity + was created, but no PublishableEntityVersion was ever made for it, so + there was never a Draft version. + 2. A Draft entry exists and points to a PublishableEntityVersion: This is + the most common state. + 3. A Draft entry exists and points to a null version: This means a version + used to be the draft, but it's been functionally "deleted". The versions + still exist in our history, but we're done using it. + + It would have saved a little space to add this data to the Published model + (and possibly call the combined model something else). Split Modulestore did + this with its active_versions table. I keep it separate here to get a better + separation of lifecycle events: i.e. this table *only* changes when drafts + are updated, not when publishing happens. The Published model only changes + when something is published. + """ + # If we're removing a PublishableEntity entirely, also remove the Draft + # entry for it. This isn't a normal operation, but can happen if you're + # deleting an entire LearningPackage. + entity = models.OneToOneField( + PublishableEntity, + on_delete=models.CASCADE, + primary_key=True, + ) + version = models.OneToOneField( + PublishableEntityVersion, + on_delete=models.RESTRICT, + null=True, + blank=True, + ) + + +class DraftChangeLog(models.Model): + """ + There is one row in this table for every time Drafts are created/modified. + + Most of the time we'll only be changing one Draft at a time, and this will + be 1:1 with DraftChange. But there are some operations that affect many + Drafts at once, such as discarding changes (i.e. reset to the published + version) or doing an import. + """ + uuid = immutable_uuid_field() + learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + changed_at = manual_date_time_field() + changed_by = models.ForeignKey( + settings.AUTH_USER_MODEL, + on_delete=models.SET_NULL, + null=True, + blank=True, + ) + + @property + def changes(self): + """ + Alias for "records". + + We use "records" for consistency with PublishLogRecord, but somteimes + it's more natural to call these "changes". + """ + return self.records + + class Meta: + verbose_name = "Draft Change Log" + verbose_name_plural = "Draft Change Logs" + + +class DraftChangeLogRecord(models.Model): + """ + A single change in the Draft version of a Publishable Entity. + + We have one unusual convention here, which is that if we have a + DraftChangeLogRecord where the old_version == new_version, it means that a + Draft's defined version hasn't changed, but the data associated with the + Draft has changed because some other entity has changed. + + The main example we have of this are containers. Imagine that we have a + Unit that is at version 1, and has unpinned references to various Components + that are its children. The Unit's version does not get incremented when the + Components are edited, because the Unit container is defined to always get + the most recent version of those Components. We would only make a new + version of the Unit if we changed the metadata of the Unit itself (e.g. the + title), or if we added, removed, or reordered the children. + + Yet updating a Component intuitively changes what we think of as the content + of the Unit. Users who are working on Units also expect that a change to a + Component will be reflected when looking at a Unit's "last updated" info. + The old_version == new_version convention lets us represent that in a useful + way because that Unit *is* a part of the change set represented by a + DraftChangeLog, even if its own versioned data hasn't changed. + """ + draft_change_log = models.ForeignKey( + DraftChangeLog, + on_delete=models.CASCADE, + related_name="records", + ) + entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) + old_version = models.ForeignKey( + PublishableEntityVersion, + on_delete=models.RESTRICT, + null=True, + blank=True, + related_name="+", + ) + new_version = models.ForeignKey( + PublishableEntityVersion, on_delete=models.RESTRICT, null=True, blank=True + ) + + class Meta: + constraints = [ + # A PublishableEntity can have only one DraftLogRecord per DraftLog. + # You can't simultaneously change the same thing in two different + # ways, e.g. set the Draft to version 1 and version 2 at the same + # time; or delete a Draft and set it to version 2 at the same time. + models.UniqueConstraint( + fields=[ + "draft_change_log", + "entity", + ], + name="oel_dlr_uniq_dcl", + ) + ] + indexes = [ + # Entity (reverse) DraftLog Index: + # * Find the history of draft changes for a given entity, starting + # with the most recent (since IDs are ascending ints). + models.Index( + fields=["entity", "-draft_change_log"], + name="oel_dlr_idx_entity_rdcl", + ), + ] + verbose_name = "Draft Log" + verbose_name_plural = "Draft Log" + + +class DraftSideEffect(models.Model): + """ + Model to track when a change in one Draft affects other Drafts. + + Our first use case for this is that changes involving child components are + thought to affect parent Units, even if the parent's version doesn't change. + + Side-effects are recorded in a collapsed form that only captures one level. + So if Components C1 and C2 are both changed and they are part of Unit U1, + which is in turn a part of Subsection SS1, then the DraftSideEffect entries + are:: + + (C1, U1) + (C2, U1) + (U1, SS1) + + We do not keep entries for (C1, SS1) or (C2, SS1). This is to make the model + simpler, so we don't have to differentiate between direct side-effects and + transitive side-effects in the model. + """ + cause = models.ForeignKey( + DraftChangeLogRecord, + on_delete=models.RESTRICT, + related_name='causes', + ) + effect = models.ForeignKey( + DraftChangeLogRecord, + on_delete=models.RESTRICT, + related_name='caused_by', + ) + + class Meta: + constraints = [ + # Duplicate entries for cause & effect are just redundant. This is + # here to guard against weird bugs that might introduce this state. + models.UniqueConstraint( + fields=["cause", "effect"], + name="oel_pub_dse_uniq_c_e", + ) + ] diff --git a/openedx_learning/apps/authoring/publishing/models/draft_published.py b/openedx_learning/apps/authoring/publishing/models/draft_published.py deleted file mode 100644 index c945d807d..000000000 --- a/openedx_learning/apps/authoring/publishing/models/draft_published.py +++ /dev/null @@ -1,95 +0,0 @@ -""" -Draft and Published models -""" -from django.db import models - -from .publish_log import PublishLogRecord -from .publishable_entity import PublishableEntity, PublishableEntityVersion - - -class Draft(models.Model): - """ - Find the active draft version of an entity (usually most recently created). - - This model mostly only exists to allow us to join against a bunch of - PublishableEntity objects at once and get all their latest drafts. You might - use this together with Published in order to see which Drafts haven't been - published yet. - - A Draft entry should be created whenever a new PublishableEntityVersion is - created. This means there are three possible states: - - 1. No Draft entry for a PublishableEntity: This means a PublishableEntity - was created, but no PublishableEntityVersion was ever made for it, so - there was never a Draft version. - 2. A Draft entry exists and points to a PublishableEntityVersion: This is - the most common state. - 3. A Draft entry exists and points to a null version: This means a version - used to be the draft, but it's been functionally "deleted". The versions - still exist in our history, but we're done using it. - - It would have saved a little space to add this data to the Published model - (and possibly call the combined model something else). Split Modulestore did - this with its active_versions table. I keep it separate here to get a better - separation of lifecycle events: i.e. this table *only* changes when drafts - are updated, not when publishing happens. The Published model only changes - when something is published. - """ - # If we're removing a PublishableEntity entirely, also remove the Draft - # entry for it. This isn't a normal operation, but can happen if you're - # deleting an entire LearningPackage. - entity = models.OneToOneField( - PublishableEntity, - on_delete=models.CASCADE, - primary_key=True, - ) - version = models.OneToOneField( - PublishableEntityVersion, - on_delete=models.RESTRICT, - null=True, - blank=True, - ) - - -class Published(models.Model): - """ - Find the currently published version of an entity. - - Notes: - - * There is only ever one published PublishableEntityVersion per - PublishableEntity at any given time. - * It may be possible for a PublishableEntity to exist only as a Draft (and thus - not show up in this table). - * If a row exists for a PublishableEntity, but the ``version`` field is - None, it means that the entity was published at some point, but is no - longer published now–i.e. it's functionally "deleted", even though all - the version history is preserved behind the scenes. - - TODO: Do we need to create a (redundant) title field in this model so that - we can more efficiently search across titles within a LearningPackage? - Probably not an immediate concern because the number of rows currently - shouldn't be > 10,000 in the more extreme cases. - - TODO: Do we need to make a "most_recently" published version when an entry - is unpublished/deleted? - """ - - entity = models.OneToOneField( - PublishableEntity, - on_delete=models.CASCADE, - primary_key=True, - ) - version = models.OneToOneField( - PublishableEntityVersion, - on_delete=models.RESTRICT, - null=True, - ) - publish_log_record = models.ForeignKey( - PublishLogRecord, - on_delete=models.RESTRICT, - ) - - class Meta: - verbose_name = "Published Entity" - verbose_name_plural = "Published Entities" diff --git a/openedx_learning/apps/authoring/publishing/models/publish_log.py b/openedx_learning/apps/authoring/publishing/models/publish_log.py index e71f08bee..037f1b8b1 100644 --- a/openedx_learning/apps/authoring/publishing/models/publish_log.py +++ b/openedx_learning/apps/authoring/publishing/models/publish_log.py @@ -104,3 +104,47 @@ class Meta: ] verbose_name = "Publish Log Record" verbose_name_plural = "Publish Log Records" + + +class Published(models.Model): + """ + Find the currently published version of an entity. + + Notes: + + * There is only ever one published PublishableEntityVersion per + PublishableEntity at any given time. + * It may be possible for a PublishableEntity to exist only as a Draft (and thus + not show up in this table). + * If a row exists for a PublishableEntity, but the ``version`` field is + None, it means that the entity was published at some point, but is no + longer published now–i.e. it's functionally "deleted", even though all + the version history is preserved behind the scenes. + + TODO: Do we need to create a (redundant) title field in this model so that + we can more efficiently search across titles within a LearningPackage? + Probably not an immediate concern because the number of rows currently + shouldn't be > 10,000 in the more extreme cases. + + TODO: Do we need to make a "most_recently" published version when an entry + is unpublished/deleted? + """ + + entity = models.OneToOneField( + PublishableEntity, + on_delete=models.CASCADE, + primary_key=True, + ) + version = models.OneToOneField( + PublishableEntityVersion, + on_delete=models.RESTRICT, + null=True, + ) + publish_log_record = models.ForeignKey( + PublishLogRecord, + on_delete=models.RESTRICT, + ) + + class Meta: + verbose_name = "Published Entity" + verbose_name_plural = "Published Entities" diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 119a55484..cfaebcf73 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -10,7 +10,7 @@ from django.core.exceptions import ValidationError from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models import LearningPackage, PublishableEntity +from openedx_learning.apps.authoring.publishing.models import LearningPackage, PublishableEntity, DraftChangeLog from openedx_learning.lib.test_utils import TestCase @@ -112,23 +112,114 @@ class DraftTestCase(TestCase): Test basic operations with Drafts. """ now: datetime - learning_package: LearningPackage + learning_package_1: LearningPackage @classmethod def setUpTestData(cls) -> None: cls.now = datetime(2024, 1, 28, 16, 45, 30, tzinfo=timezone.utc) - cls.learning_package = publishing_api.create_learning_package( - "my_package_key", - "Draft Testing LearningPackage 🔥", + cls.learning_package_1 = publishing_api.create_learning_package( + "my_package_key_1", + "Draft Testing LearningPackage 🔥 1", created=cls.now, ) + cls.learning_package_2 = publishing_api.create_learning_package( + "my_package_key_2", + "Draft Testing LearningPackage 🔥 2", + created=cls.now, + ) + + def test_simple_draft_changeset(self) -> None: + with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + 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].changes.all()) == 2 + + # Now that we're outside of the context manager, check that we're making + # a new DraftChangeSet... + entity3 = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity3", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity3.id, + version_num=1, + title="An Entity 🌴 3", + created=self.now, + created_by=None, + ) + draft_sets = list(DraftChangeLog.objects.all().order_by('id')) + assert len(draft_sets) == 2 + assert len(draft_sets[1].changes.all()) == 1 + + def test_nested_draft_changesets(self) -> None: + pass + + 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): + 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 + changes = list(draft_sets[0].changes.all()) + assert len(changes) == 1 + assert changes[0].old_version is None + assert changes[0].new_version.version_num == 2 def test_draft_lifecycle(self) -> None: """ Test basic lifecycle of a Draft. """ entity = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, "my_entity", created=self.now, created_by=None, @@ -155,7 +246,7 @@ def test_draft_lifecycle(self) -> None: def test_soft_deletes(self) -> None: """Test the publishing behavior of soft deletes.""" entity = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, "my_entity", created=self.now, created_by=None, @@ -169,7 +260,7 @@ def test_soft_deletes(self) -> None: ) # Initial publish - publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id) log_records = list(publish_log.records.all()) assert len(log_records) == 1 record = log_records[0] @@ -179,7 +270,7 @@ def test_soft_deletes(self) -> None: # Publishing the soft-delete publishing_api.soft_delete_draft(entity.id) - publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id) log_records = list(publish_log.records.all()) assert len(log_records) == 1 record = log_records[0] @@ -192,9 +283,45 @@ def test_soft_deletes(self) -> None: # all the Drafts that have different versions than their Published # counterparts" would mistakenly pull in records that were NULL in both # places. - publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id) assert publish_log.records.count() == 0 + def test_soft_delete_and_reset(self) -> None: + """ + Test edge case where we create, soft-delete, and then reset. + + This is an edge case because creating and then soft-deleting an item + sequence of actions will make both the Draft and Published version NULL. + In this situation reset_drafts_to_published should NOT create a + DraftChangeLog (because they already match, so there's nothing to do). + But we had a bug that redundantly set the Draft version to NULL again + because NULL != NULL in SQL and we were doing the Draft vs. Published + comparison naively without taking that into account. + """ + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity", + created=self.now, + created_by=None, + ) + # Draft Change #1: create the new version + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="An Entity 🌴", + created=self.now, + created_by=None, + ) + assert DraftChangeLog.objects.count() == 1 + + # Change #1: delete the draft (set the draft version to None) + publishing_api.soft_delete_draft(entity.id) + assert DraftChangeLog.objects.count() == 2 + + # This should NOT create a change: + publishing_api.reset_drafts_to_published(self.learning_package_1.id) + assert DraftChangeLog.objects.count() == 2 + def test_reset_drafts_to_published(self) -> None: """ Test throwing out Draft data and resetting to the Published versions. @@ -218,7 +345,7 @@ def test_reset_drafts_to_published(self) -> None: # 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.id, + self.learning_package_1.id, "entity_with_changes", created=self.now, created_by=None, @@ -233,7 +360,7 @@ def test_reset_drafts_to_published(self) -> None: # This Entity is going to remain unchanged between Draft and Published. entity_with_no_changes = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, "entity_with_no_changes", created=self.now, created_by=None, @@ -248,7 +375,7 @@ def test_reset_drafts_to_published(self) -> None: # This Entity will be Published, but will then be soft-deleted. entity_with_pending_delete = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, "entity_with_pending_delete", created=self.now, created_by=None, @@ -262,7 +389,7 @@ def test_reset_drafts_to_published(self) -> None: ) # Publish! - publishing_api.publish_all_drafts(self.learning_package.id) + publishing_api.publish_all_drafts(self.learning_package_1.id) # Create versions 2, 3, 4 of entity_with_changes. After this, the # Published version is 1 and the Draft version is 4. @@ -282,7 +409,7 @@ def test_reset_drafts_to_published(self) -> None: # Create a new entity that only exists in Draft form (no Published # version). new_entity = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, "new_entity", created=self.now, created_by=None, @@ -310,7 +437,7 @@ def test_reset_drafts_to_published(self) -> None: assert draft_version_num == self._get_draft_version_num(entity) # Now reset to draft here! - publishing_api.reset_drafts_to_published(self.learning_package.id) + publishing_api.reset_drafts_to_published(self.learning_package_1.id) # Versions we expect after reset in (entity, published version num) # tuples. The only entity that is not version 1 is the new one. @@ -327,10 +454,16 @@ def test_reset_drafts_to_published(self) -> None: pub_version_num ) + def test_reset_drafts_to_published_bulk(self) -> None: + """bulk_draft_changes_for creates only one DraftChangeLog.""" + with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + self.test_reset_drafts_to_published() + assert DraftChangeLog.objects.count() == 1 + 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.id, + self.learning_package_1.id, "my_entity", created=self.now, created_by=None, @@ -344,26 +477,26 @@ def test_get_entities_with_unpublished_changes(self) -> None: ) # Fetch unpublished entities - entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package.id) + entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package_1.id) records = list(entities.all()) assert len(records) == 1 record = records[0] assert record.id == entity.id # Initial publish - publishing_api.publish_all_drafts(self.learning_package.id) + publishing_api.publish_all_drafts(self.learning_package_1.id) # soft-delete entity publishing_api.soft_delete_draft(entity.id) - entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package.id) + entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package_1.id) assert len(entities) == 0 - entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package.id, + entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package_1.id, include_deleted_drafts=True) assert len(entities) == 1 # publish soft-delete - publishing_api.publish_all_drafts(self.learning_package.id) - entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package.id, + publishing_api.publish_all_drafts(self.learning_package_1.id) + entities = publishing_api.get_entities_with_unpublished_changes(self.learning_package_1.id, include_deleted_drafts=True) # should not return published soft-deleted entities. assert len(entities) == 0 @@ -376,7 +509,7 @@ def test_filter_publishable_entities(self) -> None: for index in range(count_published): # Create entities to publish entity = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, f"entity_published_{index}", created=self.now, created_by=None, @@ -390,12 +523,12 @@ def test_filter_publishable_entities(self) -> None: created_by=None, ) - publishing_api.publish_all_drafts(self.learning_package.id) + 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.id, + self.learning_package_1.id, f"entity_draft_{index}", created=self.now, created_by=None, @@ -412,7 +545,7 @@ def test_filter_publishable_entities(self) -> None: for index in range(count_no_drafts): # Create entities without drafts entity = publishing_api.create_publishable_entity( - self.learning_package.id, + self.learning_package_1.id, f"entity_no_draft_{index}", created=self.now, created_by=None, diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index c482f9375..9d0ca8873 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -171,9 +171,9 @@ def test_create_unit_queries(self): Test how many database queries are required to create a unit """ # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with self.assertNumQueries(22): + with self.assertNumQueries(25): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(26): + with self.assertNumQueries(28): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2") From 361e27c070d1240e480e11636b95eaa07233b628 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 00:04:05 -0400 Subject: [PATCH 02/29] test: add container/child draft change log tests --- .../apps/authoring/publishing/api.py | 2 +- ...aftchangelog.py => 0005_draftchangelog.py} | 2 +- .../apps/authoring/publishing/test_api.py | 167 +++++++++++++++++- 3 files changed, 168 insertions(+), 3 deletions(-) rename openedx_learning/apps/authoring/publishing/migrations/{0004_draftchangelog.py => 0005_draftchangelog.py} (98%) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index c6198bec7..724394c5a 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -638,8 +638,8 @@ def _create_container_side_effects_for_draft_change( container_change, _created = DraftChangeLogRecord.objects.get_or_create( draft_change_log=change.draft_change_log, entity_id=container.pk, - old_version_id=container_draft_version_pk, defaults={ + 'old_version_id': container_draft_version_pk, 'new_version_id': container_draft_version_pk } ) diff --git a/openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py similarity index 98% rename from openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py rename to openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py index 1ad9d905d..1fd39a912 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0004_draftchangelog.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py @@ -11,7 +11,7 @@ class Migration(migrations.Migration): dependencies = [ migrations.swappable_dependency(settings.AUTH_USER_MODEL), - ('oel_publishing', '0003_containers'), + ('oel_publishing', '0004_publishableentity_can_stand_alone'), ] operations = [ diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index cfaebcf73..dc1238689 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -10,7 +10,12 @@ from django.core.exceptions import ValidationError from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models import LearningPackage, PublishableEntity, DraftChangeLog +from openedx_learning.apps.authoring.publishing.models import ( + Container, + DraftChangeLog, + LearningPackage, + PublishableEntity, +) from openedx_learning.lib.test_utils import TestCase @@ -586,3 +591,163 @@ def _get_draft_version_num(self, entity: PublishableEntity) -> int | None: if draft_version is not None: return draft_version.version_num return None + + +class ContainerTestCase(TestCase): + """ + Test basic operations with Drafts. + """ + now: datetime + learning_package: LearningPackage + + @classmethod + def setUpTestData(cls) -> None: + cls.now = datetime(2024, 1, 28, 16, 45, 30, tzinfo=timezone.utc) + cls.learning_package = publishing_api.create_learning_package( + "containers_package_key", + "Container Testing LearningPackage 🔥 1", + created=cls.now, + ) + + def test_parent_child_side_effects(self) -> None: + """Test that modifying a child has side-effects on its parent.""" + child_1 = publishing_api.create_publishable_entity( + self.learning_package.id, + "child_1", + created=self.now, + created_by=None, + ) + child_1_v1 = publishing_api.create_publishable_entity_version( + child_1.id, + version_num=1, + title="Child 1 🌴", + created=self.now, + created_by=None, + ) + child_2 = publishing_api.create_publishable_entity( + self.learning_package.id, + "child_2", + created=self.now, + created_by=None, + ) + child_2_v1 = publishing_api.create_publishable_entity_version( + child_2.id, + version_num=1, + title="Child 2 🌴", + created=self.now, + created_by=None, + ) + container = publishing_api.create_container( + self.learning_package.id, + "my_container", + created=self.now, + created_by=None, + ) + container_v1 = publishing_api.create_container_version( + container.pk, + 1, + title="My Container", + publishable_entities_pks=[child_1.id, child_2.id], + entity_version_pks=None, + created=self.now, + created_by=None, + ) + + # All this was just set up. Now that we have our container-child + # relationships, altering a child should add the parent container to + # the DraftChangeLog. + child_1_v2 = publishing_api.create_publishable_entity_version( + child_1.id, + version_num=2, + title="Child 1 v2", + created=self.now, + created_by=None, + ) + last_change_log = DraftChangeLog.objects.order_by('-id').first() + assert last_change_log.changes.count() == 2 + child_1_change = last_change_log.changes.get(entity=child_1) + assert child_1_change.old_version == child_1_v1 + assert child_1_change.new_version == child_1_v2 + + # The container should be here, but the versions should be the same for + # before and after: + container_change = last_change_log.changes.get( + entity=container.publishable_entity + ) + assert container_change.old_version == container_v1.publishable_entity_version + assert container_change.new_version == container_v1.publishable_entity_version + + + 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): + child_1 = publishing_api.create_publishable_entity( + self.learning_package.id, + "child_1", + created=self.now, + created_by=None, + ) + child_1_v1 = publishing_api.create_publishable_entity_version( + child_1.id, + version_num=1, + title="Child 1 🌴", + created=self.now, + created_by=None, + ) + child_2 = publishing_api.create_publishable_entity( + self.learning_package.id, + "child_2", + created=self.now, + created_by=None, + ) + child_2_v1 = publishing_api.create_publishable_entity_version( + child_2.id, + version_num=1, + title="Child 2 🌴", + created=self.now, + created_by=None, + ) + container = publishing_api.create_container( + self.learning_package.id, + "my_container", + created=self.now, + created_by=None, + ) + container_v1 = publishing_api.create_container_version( + container.pk, + 1, + title="My Container", + publishable_entities_pks=[child_1.id, child_2.id], + entity_version_pks=None, + created=self.now, + created_by=None, + ) + + # All this was just set up. Now that we have our container-child + # relationships, altering a child should add the parent container to + # the DraftChangeLog. + child_1_v2 = publishing_api.create_publishable_entity_version( + child_1.id, + version_num=2, + title="Child 1 v2", + created=self.now, + created_by=None, + ) + + # Because we're doing it in bulk, there's only one DraftChangeLog entry. + assert DraftChangeLog.objects.count() == 1 + last_change_log = DraftChangeLog.objects.first() + # There's only ever one change entry per publishable entity + assert last_change_log.changes.count() == 3 + + child_1_change = last_change_log.changes.get(entity=child_1) + assert child_1_change.old_version == None + assert child_1_change.new_version == child_1_v2 + + # The container should be here, but the versions should be the same for + # before and after: + container_change = last_change_log.changes.get( + entity=container.publishable_entity + ) + assert container_change.old_version == container_v1.publishable_entity_version + assert container_change.new_version == container_v1.publishable_entity_version From c01a3a46657c7e9a6493646c44a754f53976f146 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 12:27:57 -0400 Subject: [PATCH 03/29] test: draft side effect tests and behavior writeup --- .../authoring/publishing/models/draft_log.py | 37 ++++++++++++++++++- .../apps/authoring/publishing/test_api.py | 32 +++++++++++++--- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index ab1a6ffd7..2ca658485 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -176,6 +176,41 @@ class DraftSideEffect(models.Model): We do not keep entries for (C1, SS1) or (C2, SS1). This is to make the model simpler, so we don't have to differentiate between direct side-effects and transitive side-effects in the model. + + Side-effects will also be be recorded for parent containers when children + change even if the container also changes at the same time. This is to make + it consistent so that every time a child changes it is reflected that it + affected the parent (was a child at that point in time), even if the parent + is also changing its own metadata in different ways. When we're doing this, + we only record side-effects from children if they are part of the new + version of the container. 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 + 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 + - a DraftSideEffect is created with cause (C1.v1 -> C1.v2) and effect + (U1.v1 -> U1.v1) + + Scenario 2: In a bulk_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: + - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 + - a DraftChangeLogRecord is created for U1.v1 -> U1.v2 + - 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 + 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 + - a DraftChangeLogRecord is created for U1.v1 -> U1.v2 + - no SideEffect is created, since changing C1 does not have an impact on the + current draft of U1 (U1.v2). """ cause = models.ForeignKey( DraftChangeLogRecord, @@ -185,7 +220,7 @@ class DraftSideEffect(models.Model): effect = models.ForeignKey( DraftChangeLogRecord, on_delete=models.RESTRICT, - related_name='caused_by', + related_name='affected_by', ) class Meta: diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index dc1238689..db28e73a2 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -11,8 +11,8 @@ from openedx_learning.apps.authoring.publishing import api as publishing_api from openedx_learning.apps.authoring.publishing.models import ( - Container, DraftChangeLog, + DraftSideEffect, LearningPackage, PublishableEntity, ) @@ -630,7 +630,7 @@ def test_parent_child_side_effects(self) -> None: created=self.now, created_by=None, ) - child_2_v1 = publishing_api.create_publishable_entity_version( + publishing_api.create_publishable_entity_version( child_2.id, version_num=1, title="Child 2 🌴", @@ -677,6 +677,14 @@ def test_parent_child_side_effects(self) -> None: assert container_change.old_version == container_v1.publishable_entity_version assert container_change.new_version == container_v1.publishable_entity_version + # Exactly one side-effect should have been created because we changed + # child_1 after it was part of a container. + side_effects = DraftSideEffect.objects.all() + assert side_effects.count() == 1 + side_effect = side_effects.first() + assert side_effect.cause == child_1_change + assert side_effect.effect == container_change + def test_bulk_parent_child_side_effects(self) -> None: """Test that modifying a child has side-effects on its parent. (bulk version)""" @@ -743,11 +751,23 @@ def test_bulk_parent_child_side_effects(self) -> None: child_1_change = last_change_log.changes.get(entity=child_1) assert child_1_change.old_version == None assert child_1_change.new_version == child_1_v2 - - # The container should be here, but the versions should be the same for - # before and after: + + child_2_change = last_change_log.changes.get(entity=child_2) + assert child_2_change.old_version == None + assert child_2_change.new_version == child_2_v1 + container_change = last_change_log.changes.get( entity=container.publishable_entity ) - assert container_change.old_version == container_v1.publishable_entity_version + assert container_change.old_version == 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 + # count towards side-effects on the container. + side_effects = DraftSideEffect.objects.all() + assert side_effects.count() == 2 + caused_by_child_1 = side_effects.get(cause=child_1_change) + caused_by_child_2 = side_effects.get(cause=child_2_change) + assert caused_by_child_1.effect == container_change + assert caused_by_child_2.effect == container_change From 80c8847e9e950e2d371831850498827c435d89b9 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 12:32:36 -0400 Subject: [PATCH 04/29] test: placeholder so I don't forget to write this test --- tests/openedx_learning/apps/authoring/publishing/test_api.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index db28e73a2..4a7f72897 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -771,3 +771,7 @@ def test_bulk_parent_child_side_effects(self) -> None: caused_by_child_2 = side_effects.get(cause=child_2_change) assert caused_by_child_1.effect == container_change assert caused_by_child_2.effect == container_change + + def test_multiple_layers_of_containers(self): + """Test stacking containers three layers deep.""" + pass From 3646370f0d7a46b1fa89c401ce4a9b4db1c23a8a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 12:39:30 -0400 Subject: [PATCH 05/29] feat: add migration to infer DraftChangeLogs from existing data. --- .../0006_bootstrap_draftchangelog.py | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py diff --git a/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py new file mode 100644 index 000000000..42c40eb36 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py @@ -0,0 +1,94 @@ +""" +Bootstrap DraftChangeLogs + +DraftChangeLog and DraftChangeLogRecord are being introduced after Drafts, so +we're going to retroactively make entries for all the changes that were in our +Learning Packages. + +This migration will try to reconstruct create, edit, reset-to-published, and +delete operations, but it won't be fully accurate because we only have the +create dates of the versions and the current state of active Drafts to go on. +This means we won't accurately capture when things were deleted and then reset, +or when reset and then later edited. We're also missing the user for a number of +these operations, so we'll add those with null created_by entries. Addressing +these gaps is a big part of why we created DraftChangeLogs in the first place. +""" +# Generated by Django 4.2.18 on 2025-03-13 10:29 +import logging +from datetime import datetime, timezone + +from django.db import migrations + +logger = logging.getLogger(__name__) + + +def bootstrap_draft_change_logs(apps, schema_editor): + """ + Create a fake DraftChangeSet that encompasses the state of current Drafts. + """ + LearningPackage = apps.get_model("oel_publishing", "LearningPackage") + PublishableEntityVersion = apps.get_model("oel_publishing", "PublishableEntityVersion") + + Draft = apps.get_model("oel_publishing", "Draft") + DraftChangeLogRecord = apps.get_model("oel_publishing", "DraftChangeLogRecord") + DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") + now = datetime.now(tz=timezone.utc) + + for learning_package in LearningPackage.objects.all().order_by("key"): + logger.info(f"Creating bootstrap DraftChangeLogs for {learning_package.key}") + pub_ent_versions = PublishableEntityVersion.objects.filter( + entity__learning_package=learning_package + ).select_related("entity") + + # First cycle though all the simple create/edit operations... + last_version_seen = {} # PublishableEntity.id -> PublishableEntityVersion.id + for pub_ent_version in pub_ent_versions.order_by("pk"): + draft_change_log = DraftChangeLog.objects.create( + learning_package=learning_package, + changed_at=pub_ent_version.created, + changed_by=pub_ent_version.created_by, + ) + DraftChangeLogRecord.objects.create( + draft_change_log=draft_change_log, + entity=pub_ent_version.entity, + old_version_id=last_version_seen.get(pub_ent_version.entity.id), + new_version_id=pub_ent_version.id, + ) + last_version_seen[pub_ent_version.entity.id] = pub_ent_version.id + + # Now that we've created change sets for create/edit operations, we look + # at the latest state of the Draft model in order to determine whether + # we need to apply deletes or resets. + for draft in Draft.objects.filter(entity__learning_package=learning_package).order_by("entity_id"): + last_version_id = last_version_seen.get(draft.entity_id) + if draft.version_id == last_version_id: + continue + # We don't really know who did this or when, so we use None and now. + draft_change_log = DraftChangeLog.objects.create( + learning_package=learning_package, + changed_at=now, + changed_by=None, + ) + DraftChangeLogRecord.objects.create( + draft_change_log=draft_change_log, + entity_id=draft.entity_id, + old_version_id=last_version_id, + new_version_id=draft.version_id, + ) + + +def delete_draft_change_logs(apps, schema_editor): + logger.info(f"Deleting all DraftChangeLogs (reverse migration)") + DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") + DraftChangeLog.objects.all().delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0005_draftchangelog'), + ] + + operations = [ + migrations.RunPython(bootstrap_draft_change_logs, reverse_code=delete_draft_change_logs) + ] From 2ef81cc9d944da2fa23345acbc5410952ebb89a9 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 12:43:18 -0400 Subject: [PATCH 06/29] fix: admin needed to be updated after naming refactor --- openedx_learning/apps/authoring/publishing/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index b8b351286..eaaed52bd 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -241,4 +241,4 @@ def num_changes(self, draft_change_set): def get_queryset(self, request): queryset = super().get_queryset(request) return queryset.select_related("learning_package", "changed_by") \ - .annotate(num_changes=Count("changes")) + .annotate(num_changes=Count("records")) From e7e9cc3976ed90daf6321b1998c9edcdc6ae2108 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 1 Apr 2025 13:01:55 -0400 Subject: [PATCH 07/29] temp: tried to clean up the DraftChange docstring to be more helpful --- .../authoring/publishing/models/draft_log.py | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index 2ca658485..50f8edc2f 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -177,13 +177,13 @@ class DraftSideEffect(models.Model): simpler, so we don't have to differentiate between direct side-effects and transitive side-effects in the model. - Side-effects will also be be recorded for parent containers when children - change even if the container also changes at the same time. This is to make - it consistent so that every time a child changes it is reflected that it - affected the parent (was a child at that point in time), even if the parent - is also changing its own metadata in different ways. When we're doing this, - we only record side-effects from children if they are part of the new - version of the container. To give concrete examples: + We will record side-effects on a parent container whenever a child changes, + even if the parent container is also changing in the same DraftChangeLog. + 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 + examples: Setup: A Unit version U1.v1 has defined C1 to be a child. The current draft version of C1 is C1.v1. @@ -193,7 +193,10 @@ class DraftSideEffect(models.Model): - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - a DraftChangeLogRecord is created for U1.v1 -> U1.v1 - a DraftSideEffect is created with cause (C1.v1 -> C1.v2) and effect - (U1.v1 -> U1.v1) + (U1.v1 -> U1.v1). The Unit draft version has not been incremented because + the metadata a Unit defines for itself hasn't been altered, but the Unit + 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 draft version of C1 is now C1.v2. In the same context, we edit U1's metadata @@ -210,7 +213,9 @@ class DraftSideEffect(models.Model): - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - a DraftChangeLogRecord is created for U1.v1 -> U1.v2 - no SideEffect is created, since changing C1 does not have an impact on the - current draft of U1 (U1.v2). + current draft of U1 (U1.v2). A DraftChangeLog is considered a single + atomic operation, so there was never a point at which C1.v1 -> C1.v2 + affected the draft state of U1. """ cause = models.ForeignKey( DraftChangeLogRecord, From 17192daa64b9bfc13ee6322153cbe328ce449f27 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 00:03:34 -0400 Subject: [PATCH 08/29] temp: incorporate review feedback --- .../apps/authoring/publishing/api.py | 10 +++++----- .../authoring/publishing/models/draft_log.py | 14 ++----------- .../apps/authoring/publishing/test_api.py | 20 +++++++++---------- 3 files changed, 17 insertions(+), 27 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 724394c5a..f9e2bd645 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -476,7 +476,7 @@ def set_draft_version( from Studio's editing point of view (see ``soft_delete_draft`` for more details). - Calling this function attaches a new DraftChange and attaches it to a + Calling this function attaches a new DraftChangeLogRecordand attaches it to a DraftChangeLog. This function will create DraftSideEffect entries and properly add any @@ -570,10 +570,10 @@ def _add_to_existing_draft_change_log( ) -> DraftChangeLogRecord: # If we get here, it means there is an active DraftChangeLog that may # have many DraftChanges associated with it. A DraftChangeLog can only - # have one DraftChange per PublishableEntity, e.g. the same Component + # have one DraftChangeLogRecordper PublishableEntity, e.g. the same Component # can't go from v1 to v3 and v1 to v4 in the same DraftChangeLog. try: - # If there was already a DraftChange for this PublishableEntity, + # If there was already a DraftChangeLogRecordfor this PublishableEntity, # we update the new_version_id. We keep the old_version_id value # though, because that represents what it was before this # DraftChangeLog, and we don't want to lose that information. @@ -584,7 +584,7 @@ def _add_to_existing_draft_change_log( change.new_version_id = new_version_id change.save() except DraftChangeLogRecord.DoesNotExist: - # If we're here, this is the first DraftChange we're making for + # If we're here, this is the first DraftChangeLogRecordwe're making for # this PublishableEntity in the active DraftChangeLog. change = DraftChangeLogRecord.objects.create( draft_change_log=active_change_log, @@ -598,7 +598,7 @@ def _add_to_existing_draft_change_log( def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLogRecord): processed_entity_ids = set() - for change in change_log.changes.all(): + for change in change_log.records.all(): _create_container_side_effects_for_draft_change( change, processed_entity_ids=processed_entity_ids, diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index 50f8edc2f..c201fb1a3 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -60,8 +60,8 @@ class DraftChangeLog(models.Model): There is one row in this table for every time Drafts are created/modified. Most of the time we'll only be changing one Draft at a time, and this will - be 1:1 with DraftChange. But there are some operations that affect many - Drafts at once, such as discarding changes (i.e. reset to the published + be 1:1 with DraftChangeLogRecord. But there are some operations that affect + many Drafts at once, such as discarding changes (i.e. reset to the published version) or doing an import. """ uuid = immutable_uuid_field() @@ -74,16 +74,6 @@ class DraftChangeLog(models.Model): blank=True, ) - @property - def changes(self): - """ - Alias for "records". - - We use "records" for consistency with PublishLogRecord, but somteimes - it's more natural to call these "changes". - """ - return self.records - class Meta: verbose_name = "Draft Change Log" verbose_name_plural = "Draft Change Logs" diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 4a7f72897..2ebf8592b 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -163,7 +163,7 @@ def test_simple_draft_changeset(self) -> None: ) draft_sets = list(DraftChangeLog.objects.all()) assert len(draft_sets) == 1 - assert len(draft_sets[0].changes.all()) == 2 + assert len(draft_sets[0].records.all()) == 2 # Now that we're outside of the context manager, check that we're making # a new DraftChangeSet... @@ -182,7 +182,7 @@ def test_simple_draft_changeset(self) -> None: ) draft_sets = list(DraftChangeLog.objects.all().order_by('id')) assert len(draft_sets) == 2 - assert len(draft_sets[1].changes.all()) == 1 + assert len(draft_sets[1].records.all()) == 1 def test_nested_draft_changesets(self) -> None: pass @@ -214,7 +214,7 @@ def test_multiple_draft_changes(self) -> None: ) draft_sets = list(DraftChangeLog.objects.all().order_by('id')) assert len(draft_sets) == 1 - changes = list(draft_sets[0].changes.all()) + changes = list(draft_sets[0].records.all()) assert len(changes) == 1 assert changes[0].old_version is None assert changes[0].new_version.version_num == 2 @@ -664,14 +664,14 @@ def test_parent_child_side_effects(self) -> None: created_by=None, ) last_change_log = DraftChangeLog.objects.order_by('-id').first() - assert last_change_log.changes.count() == 2 - child_1_change = last_change_log.changes.get(entity=child_1) + assert last_change_log.records.count() == 2 + child_1_change = last_change_log.records.get(entity=child_1) assert child_1_change.old_version == child_1_v1 assert child_1_change.new_version == child_1_v2 # The container should be here, but the versions should be the same for # before and after: - container_change = last_change_log.changes.get( + container_change = last_change_log.records.get( entity=container.publishable_entity ) assert container_change.old_version == container_v1.publishable_entity_version @@ -746,17 +746,17 @@ def test_bulk_parent_child_side_effects(self) -> None: assert DraftChangeLog.objects.count() == 1 last_change_log = DraftChangeLog.objects.first() # There's only ever one change entry per publishable entity - assert last_change_log.changes.count() == 3 + assert last_change_log.records.count() == 3 - child_1_change = last_change_log.changes.get(entity=child_1) + child_1_change = last_change_log.records.get(entity=child_1) assert child_1_change.old_version == None assert child_1_change.new_version == child_1_v2 - child_2_change = last_change_log.changes.get(entity=child_2) + child_2_change = last_change_log.records.get(entity=child_2) assert child_2_change.old_version == None assert child_2_change.new_version == child_2_v1 - container_change = last_change_log.changes.get( + container_change = last_change_log.records.get( entity=container.publishable_entity ) assert container_change.old_version == None From 6cc7dc263422d8a24dcdfc92c2042eadac042fba Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 00:48:18 -0400 Subject: [PATCH 09/29] temp: assorted pylint/mypy fixups, because my brain isn't working well enough for anything else at the moment --- .../apps/authoring/publishing/admin.py | 9 ++-- .../apps/authoring/publishing/api.py | 42 +++++++++++++------ .../authoring/publishing/contextmanagers.py | 31 +++++++++++--- .../authoring/publishing/models/draft_log.py | 30 ++++++++----- .../apps/authoring/publishing/test_api.py | 17 ++++---- 5 files changed, 88 insertions(+), 41 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index eaaed52bd..d6673ee5d 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -6,8 +6,8 @@ from django.contrib import admin from django.db.models import Count -from .models.publish_log import Published from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin, one_to_one_related_model_html +from .models.publish_log import Published from .models import ( DraftChangeLogRecord, @@ -181,7 +181,10 @@ def message(self, published_obj): return published_obj.publish_log_record.publish_log.message -class DraftChangeTabularInline(admin.TabularInline): +class DraftChangeLogRecordTabularInline(admin.TabularInline): + """ + Tabular inline for a single Draft change. + """ model = DraftChangeLogRecord fields = ( @@ -223,7 +226,7 @@ class DraftChangeSetAdmin(ReadOnlyModelAdmin): """ Read-only admin to view Draft changes (via inline tables) """ - inlines = [DraftChangeTabularInline] + inlines = [DraftChangeLogRecordTabularInline] fields = ( "uuid", "learning_package", diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index f9e2bd645..ca1f590b4 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -11,7 +11,7 @@ from datetime import datetime, timezone from enum import Enum from functools import singledispatch -from typing import TypeVar +from typing import ContextManager, TypeVar from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet @@ -488,7 +488,7 @@ def set_draft_version( """ if set_at is None: set_at = datetime.now(tz=timezone.utc) - + tx_context = atomic() if create_transaction else nullcontext() with tx_context: @@ -533,7 +533,7 @@ def set_draft_version( entity_id=draft.entity_id, old_version_id=old_version_id, new_version_id=publishable_entity_version_pk, - ) + ) _create_container_side_effects_for_draft_change(change) @@ -568,12 +568,24 @@ def _add_to_existing_draft_change_log( old_version_id: int | None, new_version_id: int | None, ) -> DraftChangeLogRecord: - # If we get here, it means there is an active DraftChangeLog that may - # have many DraftChanges associated with it. A DraftChangeLog can only - # have one DraftChangeLogRecordper PublishableEntity, e.g. the same Component - # can't go from v1 to v3 and v1 to v4 in the same DraftChangeLog. + """ + Create or update a DraftChangeLogRecord for the active_change_log passed in. + + The an active_change_log may have many DraftChangeLogRecords already + associated with it. A DraftChangeLog can only have one DraftChangeLogRecord + per PublishableEntity, e.g. the same Component can't go from v1 to v2 and v2 + to v3 in the same DraftChangeLog. The DraftChangeLogRecord is meant to + capture the before and after states of the Draft version for that entity, + so we always keep the first value for old_version, while updating to the + most recent value for new_version. + + So for example, if we called this function with the same active_change_log + and the same entity_id but with versions: (None, v1), (v1, v2), (v2, v3); + we would collapse them into one DraftChangeLogrecord with old_version = None + and new_version = v3. + """ try: - # If there was already a DraftChangeLogRecordfor this PublishableEntity, + # If there is already a DraftChangeLogRecord for this PublishableEntity, # we update the new_version_id. We keep the old_version_id value # though, because that represents what it was before this # DraftChangeLog, and we don't want to lose that information. @@ -584,8 +596,8 @@ def _add_to_existing_draft_change_log( change.new_version_id = new_version_id change.save() except DraftChangeLogRecord.DoesNotExist: - # If we're here, this is the first DraftChangeLogRecordwe're making for - # this PublishableEntity in the active DraftChangeLog. + # If we're here, this is the first DraftChangeLogRecord we're making for + # this PublishableEntity in the active DraftChangeLog. change = DraftChangeLogRecord.objects.create( draft_change_log=active_change_log, entity_id=entity_id, @@ -596,8 +608,11 @@ def _add_to_existing_draft_change_log( return change -def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLogRecord): - processed_entity_ids = set() +def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLog): + """ + Iterate through the whole DraftChangeLog and process side-effects. + """ + processed_entity_ids: set[int] = set() for change in change_log.records.all(): _create_container_side_effects_for_draft_change( change, @@ -740,6 +755,7 @@ def reset_drafts_to_published( # 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: @@ -1385,7 +1401,7 @@ def get_container_children_count( return container_version.entity_list.entitylistrow_set.filter(**filter_deleted).count() -def bulk_draft_changes_for(learning_package_id: int, changed_by=None, changed_at=None) -> DraftChangeLog: +def bulk_draft_changes_for(learning_package_id: int, changed_by=None, changed_at=None) -> DraftChangeLogContext: """ Context manager to do a single batch of Draft changes in. """ diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py index 32e628bf0..a237b4947 100644 --- a/openedx_learning/apps/authoring/publishing/contextmanagers.py +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -1,3 +1,9 @@ +""" +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). +""" from contextvars import ContextVar from datetime import datetime, timezone @@ -43,15 +49,18 @@ class to essentially keep the active DraftChangeChangeLog in a global an atomic operation. """ _draft_change_logs: ContextVar[list | None] = ContextVar('_draft_change_logs', default=None) - + def __init__(self, learning_package_id, changed_at=None, changed_by=None, exit_callbacks=None): super().__init__(using=None, savepoint=False, durable=False) self.learning_package_id = learning_package_id self.changed_by = changed_by self.changed_at = changed_at or datetime.now(tz=timezone.utc) - - # We currently use self.exit_callbacks as a way to ruh parent/child + + # This will get properly initialized on __enter__() + self.draft_change_log = None + + # We currently use self.exit_callbacks as a way to run parent/child # side-effect creation. DraftChangeLogContext itself is a lower-level # part of the code that doesn't understand what containers are. self.exit_callbacks = exit_callbacks or [] @@ -93,6 +102,11 @@ def get_active_draft_change_log(cls, learning_package_id: int) -> DraftChangeLog return None def __enter__(self): + """ + Enter our context. + + This starts the transaction and sets up our active DraftChangeLog. + """ super().__enter__() self.draft_change_log = DraftChangeLog.objects.create( @@ -108,7 +122,14 @@ def __enter__(self): return self.draft_change_log - def __exit__(self, type, value, traceback): + def __exit__(self, exc_type, exc_value, traceback): + """ + Exit our context. + + Pops the active DraftChangeLog off of our stack and run any + post-processing callbacks needed. This callback mechanism is how child- + parent side-effects are calculated. + """ draft_change_logs = self._draft_change_logs.get() if draft_change_logs: draft_change_log = draft_change_logs.pop() @@ -117,4 +138,4 @@ def __exit__(self, type, value, traceback): self._draft_change_logs.set(draft_change_logs) - return super().__exit__(type, value, traceback) + return super().__exit__(exc_type, exc_value, traceback) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index c201fb1a3..49752fdd5 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -59,10 +59,16 @@ class DraftChangeLog(models.Model): """ There is one row in this table for every time Drafts are created/modified. - Most of the time we'll only be changing one Draft at a time, and this will - be 1:1 with DraftChangeLogRecord. But there are some operations that affect - many Drafts at once, such as discarding changes (i.e. reset to the published - version) or doing an import. + There are some operations that affect many Drafts at once, such as + discarding changes (i.e. reset to the published versions) or doing an + import. These would be represented by one DraftChangeLog with many + DraftChangeLogRecords in it--one DraftChangeLogRecord for every + PublishableEntity that was modified. + + Even if we're only directly changing the draft version of one + PublishableEntity, we will get multiple DraftChangeLogRecords if changing + that entity causes side-effects. See the docstrings for DraftChangeLogRecord + and DraftSideEffect for more details. """ uuid = immutable_uuid_field() learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) @@ -75,8 +81,8 @@ class DraftChangeLog(models.Model): ) class Meta: - verbose_name = "Draft Change Log" - verbose_name_plural = "Draft Change Logs" + verbose_name = _("Draft Change Log") + verbose_name_plural = _("Draft Change Logs") class DraftChangeLogRecord(models.Model): @@ -84,7 +90,7 @@ class DraftChangeLogRecord(models.Model): A single change in the Draft version of a Publishable Entity. We have one unusual convention here, which is that if we have a - DraftChangeLogRecord where the old_version == new_version, it means that a + DraftChangeLogRecord where the old_version == new_version, it means that a Draft's defined version hasn't changed, but the data associated with the Draft has changed because some other entity has changed. @@ -143,8 +149,8 @@ class Meta: name="oel_dlr_idx_entity_rdcl", ), ] - verbose_name = "Draft Log" - verbose_name_plural = "Draft Log" + verbose_name = _("Draft Change Log Record") + verbose_name_plural = _("Draft Change Log Records") class DraftSideEffect(models.Model): @@ -186,7 +192,7 @@ class DraftSideEffect(models.Model): (U1.v1 -> U1.v1). The Unit draft version has not been incremented because the metadata a Unit defines for itself hasn't been altered, but the Unit has *changed* in some way because of the side effect of its child being - edited. + edited. Scenario 2: In a bulk_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 @@ -221,9 +227,11 @@ class DraftSideEffect(models.Model): class Meta: constraints = [ # Duplicate entries for cause & effect are just redundant. This is - # here to guard against weird bugs that might introduce this state. + # here to guard against weird bugs that might introduce this state. models.UniqueConstraint( fields=["cause", "effect"], name="oel_pub_dse_uniq_c_e", ) ] + verbose_name = _("Draft Side Effect") + verbose_name_plural = _("Draft Side Effects") diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 2ebf8592b..bc3bcf0af 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -14,7 +14,7 @@ DraftChangeLog, DraftSideEffect, LearningPackage, - PublishableEntity, + PublishableEntity, ) from openedx_learning.lib.test_utils import TestCase @@ -615,7 +615,7 @@ def test_parent_child_side_effects(self) -> None: self.learning_package.id, "child_1", created=self.now, - created_by=None, + created_by=None, ) child_1_v1 = publishing_api.create_publishable_entity_version( child_1.id, @@ -668,7 +668,7 @@ def test_parent_child_side_effects(self) -> None: child_1_change = last_change_log.records.get(entity=child_1) assert child_1_change.old_version == child_1_v1 assert child_1_change.new_version == child_1_v2 - + # The container should be here, but the versions should be the same for # before and after: container_change = last_change_log.records.get( @@ -693,9 +693,9 @@ def test_bulk_parent_child_side_effects(self) -> None: self.learning_package.id, "child_1", created=self.now, - created_by=None, + created_by=None, ) - child_1_v1 = publishing_api.create_publishable_entity_version( + publishing_api.create_publishable_entity_version( child_1.id, version_num=1, title="Child 1 🌴", @@ -749,17 +749,17 @@ def test_bulk_parent_child_side_effects(self) -> None: assert last_change_log.records.count() == 3 child_1_change = last_change_log.records.get(entity=child_1) - assert child_1_change.old_version == None + assert child_1_change.old_version is None assert child_1_change.new_version == child_1_v2 child_2_change = last_change_log.records.get(entity=child_2) - assert child_2_change.old_version == None + assert child_2_change.old_version is None assert child_2_change.new_version == child_2_v1 container_change = last_change_log.records.get( entity=container.publishable_entity ) - assert container_change.old_version == None + assert container_change.old_version is None assert container_change.new_version == container_v1.publishable_entity_version # There are two side effects here, because we grouped our draft edits @@ -774,4 +774,3 @@ def test_bulk_parent_child_side_effects(self) -> None: def test_multiple_layers_of_containers(self): """Test stacking containers three layers deep.""" - pass From 7e5b928f2f5fa9c14ef1e847433e090f5149fb29 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 18:02:30 -0400 Subject: [PATCH 10/29] docs: rewrite docstring for DraftChangeLog to be more comprehensive --- .../authoring/publishing/models/draft_log.py | 112 ++++++++++++++---- 1 file changed, 91 insertions(+), 21 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index 49752fdd5..3b2bb1701 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -87,27 +87,97 @@ class Meta: class DraftChangeLogRecord(models.Model): """ - A single change in the Draft version of a Publishable Entity. - - We have one unusual convention here, which is that if we have a - DraftChangeLogRecord where the old_version == new_version, it means that a - Draft's defined version hasn't changed, but the data associated with the - Draft has changed because some other entity has changed. - - The main example we have of this are containers. Imagine that we have a - Unit that is at version 1, and has unpinned references to various Components - that are its children. The Unit's version does not get incremented when the - Components are edited, because the Unit container is defined to always get - the most recent version of those Components. We would only make a new - version of the Unit if we changed the metadata of the Unit itself (e.g. the - title), or if we added, removed, or reordered the children. - - Yet updating a Component intuitively changes what we think of as the content - of the Unit. Users who are working on Units also expect that a change to a - Component will be reflected when looking at a Unit's "last updated" info. - The old_version == new_version convention lets us represent that in a useful - way because that Unit *is* a part of the change set represented by a - DraftChangeLog, even if its own versioned data hasn't changed. + A single change in the PublishableEntity that Draft points to. + + Within a single DraftChangeLog, there can be only one DraftChangeLogRecord + per PublishableEntity. If a PublishableEntity goes from v1 -> v2 and then v2 + -> v3 within the same DraftChangeLog, the expectation is that these will be + collapsed into one DraftChangeLogRecord that goes from v1 -> v3. A single + PublishableEntity may have many DraftChangeLogRecords that describe its full + draft edit history, but each DraftChangeLogRecord will be a part of a + different DraftChangeLog. + + New PublishableEntityVersions are created with a monotonically increasing + version_num for their PublishableEntity. However, knowing that is not enough + to accurately reconstruct how the Draft changes over time because the Draft + does not always point to the most recently created PublishableEntityVersion. + We also have the concept of side-effects, where we consider a + PublishableEntity to have changed in some way, even if no new version is + explicitly created. + + The following scenarios may occur: + + Scenario 1: old_version is None, new_version.version_num = 1 + + This is the common case when we're creating the first version for editing. + + Scenario 2: old_version.version_num + 1 == new_version.version_num + + This is the common case when we've made an edit to something, which + creates the next version of an entity, which we then point the Draft at. + + Scenario 3: old_version.version_num >=1, new_version is None + + This is a soft-deletion. We never actually delete a row from the + PublishableEntity model, but set its current Draft version to be None + instead. + + Scenario 4: old_version.version_num > new_version.version_num + + This can happen if we "discard changes", meaning that we call + reset_drafts_to_published(). The versions we created before resetting + don't get deleted, but the Draft model's pointer to the current version + has been reset to match the Published model. + + Scenario 5: old_version.version_num <<< new_version.version_num + + Sometimes we'll have a gap between the two version numbers that is > 1. + This can happen if we make edits (new versions) after we called + reset_drafts_to_published. PublishableEntityVersions are created with a + monotonically incrementing version_num which will continue to go up with + the next edit, regardless of whether Draft is pointing to the most + recently created version or not. In terms of (old_version, new version) + changes, it could look like this: + + - (None, v1): Initial creation + - # Publish happens here, so v1 of this PublishableEntity is published. + - (v1, v2): Normal edit in draft + - (v2, v3): Normal edit in draft + - (v3, v1): Reset to published happened here. + - (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 + them into the same DraftChangeLog, which forces us to squash the changes + together into one DraftChangeLogRecord. + + Scenario 6: old_version is None, new_version > 1 + + This edge case can happen if we soft-deleted a published entity, and then + called reset_drafts_to_published before we published that soft-deletion. + It would effectively undo our soft-delete because the published version + was not yet marked as deleted. + + Scenario 7: old_version == new_version + + This means that the data associated with the Draft version of an entity + has changed purely as a side-effect of some other entity changing. + + The main example we have of this are containers. Imagine that we have a + Unit that is at v1, and has unpinned references to various Components that + are its children. The Unit's version does not get incremented when the + Components are edited, because the Unit container is defined to always get + the most recent version of those Components. We would only make a new + version of the Unit if we changed the metadata of the Unit itself (e.g. + the title), or if we added, removed, or reordered the children. + + Yet updating a Component intuitively changes what we think of as the + content of the Unit. Users who are working on Units also expect that a + change to a Component will be reflected when looking at a Unit's + "last updated" info. The old_version == new_version convention lets us + represent that in a useful way because that Unit *is* a part of the change + set represented by a DraftChangeLog, even if its own versioned data hasn't + changed. """ draft_change_log = models.ForeignKey( DraftChangeLog, From 5023ebd1c516c475bf4fbbd1b3b0b98af7468de8 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 18:07:50 -0400 Subject: [PATCH 11/29] temp: be more specific in notation --- openedx_learning/apps/authoring/publishing/models/draft_log.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index 3b2bb1701..2daf6f6c1 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -129,7 +129,7 @@ class DraftChangeLogRecord(models.Model): don't get deleted, but the Draft model's pointer to the current version has been reset to match the Published model. - Scenario 5: old_version.version_num <<< new_version.version_num + Scenario 5: old_version.version_num + 1 < new_version.version_num Sometimes we'll have a gap between the two version numbers that is > 1. This can happen if we make edits (new versions) after we called From 554b265c55751c90d011e3965bbe3d602b3c3375 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 22:42:03 -0400 Subject: [PATCH 12/29] test: fix mypy errors --- .../apps/authoring/publishing/api.py | 18 +++++++++++++++++ .../apps/authoring/publishing/test_api.py | 20 +++++++++++++------ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index ca1f590b4..b96b2e8cb 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -460,6 +460,24 @@ def get_published_version(publishable_entity_id: int, /) -> PublishableEntityVer @singledispatch def set_draft_version( + arg: object, + publishable_entity_version_pk: int | None, + /, + set_at: datetime | None = None, + set_by: int | None = None, # User.id + create_transaction: bool = True, +) -> None: + """ + The fallback for singledispatch is supposed to take a superclass of the + different registered versions (Draft and int in this case). This is that + fallback, but the "real" implementation is the Draft-registered version + below. + """ + raise NotImplementedError() + + +@set_draft_version.register(Draft) +def _( draft: Draft, publishable_entity_version_pk: int | None, /, diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index bc3bcf0af..390d650ee 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -11,6 +11,8 @@ from openedx_learning.apps.authoring.publishing import api as publishing_api from openedx_learning.apps.authoring.publishing.models import ( + Container, + ContainerVersion, DraftChangeLog, DraftSideEffect, LearningPackage, @@ -118,6 +120,7 @@ class DraftTestCase(TestCase): """ now: datetime learning_package_1: LearningPackage + learning_package_2: LearningPackage @classmethod def setUpTestData(cls) -> None: @@ -216,8 +219,10 @@ def test_multiple_draft_changes(self) -> None: assert len(draft_sets) == 1 changes = list(draft_sets[0].records.all()) assert len(changes) == 1 - assert changes[0].old_version is None - assert changes[0].new_version.version_num == 2 + change = changes[0] + assert change.old_version is None + assert change.new_version is not None + assert change.new_version.version_num == 2 def test_draft_lifecycle(self) -> None: """ @@ -637,13 +642,13 @@ def test_parent_child_side_effects(self) -> None: created=self.now, created_by=None, ) - container = publishing_api.create_container( + container: Container = publishing_api.create_container( self.learning_package.id, "my_container", created=self.now, created_by=None, ) - container_v1 = publishing_api.create_container_version( + container_v1: ContainerVersion = publishing_api.create_container_version( container.pk, 1, title="My Container", @@ -664,6 +669,7 @@ def test_parent_child_side_effects(self) -> None: created_by=None, ) last_change_log = DraftChangeLog.objects.order_by('-id').first() + assert last_change_log is not None assert last_change_log.records.count() == 2 child_1_change = last_change_log.records.get(entity=child_1) assert child_1_change.old_version == child_1_v1 @@ -682,6 +688,7 @@ def test_parent_child_side_effects(self) -> None: side_effects = DraftSideEffect.objects.all() assert side_effects.count() == 1 side_effect = side_effects.first() + assert side_effect is not None assert side_effect.cause == child_1_change assert side_effect.effect == container_change @@ -715,13 +722,13 @@ def test_bulk_parent_child_side_effects(self) -> None: created=self.now, created_by=None, ) - container = publishing_api.create_container( + container: Container = publishing_api.create_container( self.learning_package.id, "my_container", created=self.now, created_by=None, ) - container_v1 = publishing_api.create_container_version( + container_v1: ContainerVersion = publishing_api.create_container_version( container.pk, 1, title="My Container", @@ -745,6 +752,7 @@ def test_bulk_parent_child_side_effects(self) -> None: # Because we're doing it in bulk, there's only one DraftChangeLog entry. assert DraftChangeLog.objects.count() == 1 last_change_log = DraftChangeLog.objects.first() + assert last_change_log is not None # There's only ever one change entry per publishable entity assert last_change_log.records.count() == 3 From 7f558aaf3fdc1f798e8ba7fcaa696d2431b139a7 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 22:45:08 -0400 Subject: [PATCH 13/29] test: more linting fixes --- openedx_learning/apps/authoring/publishing/admin.py | 3 ++- openedx_learning/apps/authoring/publishing/contextmanagers.py | 4 ++-- tests/openedx_learning/apps/authoring/publishing/test_api.py | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index d6673ee5d..8af40c505 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -18,6 +18,7 @@ PublishLogRecord, ) + @admin.register(LearningPackage) class LearningPackageAdmin(ReadOnlyModelAdmin): """ @@ -236,7 +237,7 @@ class DraftChangeSetAdmin(ReadOnlyModelAdmin): ) readonly_fields = fields list_display = fields - list_filter = ["learning_package",] + list_filter = ["learning_package"] def num_changes(self, draft_change_set): return draft_change_set.num_changes diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py index a237b4947..19b9d048b 100644 --- a/openedx_learning/apps/authoring/publishing/contextmanagers.py +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -20,7 +20,7 @@ class DraftChangeLogContext(Atomic): should use the bulk_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. - + The main idea is that we want to create a context manager that will keep track of what the "active" DraftChangeChangeLogs is for a given LearningPackage. The problem is that declaring the context can happen many @@ -46,7 +46,7 @@ class to essentially keep the active DraftChangeChangeLog in a global DraftChangeLogContext also subclasses Django's Atomic context manager, since any operation on multiple Drafts as part of a DraftChangeLog will want to be - an atomic operation. + an atomic operation. """ _draft_change_logs: ContextVar[list | None] = ContextVar('_draft_change_logs', default=None) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 390d650ee..75aebde1c 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -692,7 +692,6 @@ def test_parent_child_side_effects(self) -> None: assert side_effect.cause == child_1_change assert side_effect.effect == container_change - 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): From f6d61929b212c5aa25bfe2ff11a6cd209745bae4 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 2 Apr 2025 22:46:24 -0400 Subject: [PATCH 14/29] chore: import sorting --- openedx_learning/apps/authoring/publishing/admin.py | 4 ++-- openedx_learning/apps/authoring/publishing/api.py | 5 ++--- .../authoring/publishing/migrations/0005_draftchangelog.py | 6 ++++-- .../apps/authoring/publishing/models/__init__.py | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index 8af40c505..a710536c8 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -7,16 +7,16 @@ from django.db.models import Count from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin, one_to_one_related_model_html -from .models.publish_log import Published from .models import ( - DraftChangeLogRecord, DraftChangeLog, + DraftChangeLogRecord, LearningPackage, PublishableEntity, PublishLog, PublishLogRecord, ) +from .models.publish_log import Published @admin.register(LearningPackage) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index b96b2e8cb..0c445a509 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -17,15 +17,13 @@ from django.db.models import F, Q, QuerySet from django.db.transaction import atomic -from .models.publish_log import Published - from .contextmanagers import DraftChangeLogContext from .models import ( Container, ContainerVersion, Draft, - DraftChangeLogRecord, DraftChangeLog, + DraftChangeLogRecord, DraftSideEffect, EntityList, EntityListRow, @@ -38,6 +36,7 @@ PublishLog, PublishLogRecord, ) +from .models.publish_log import Published # A few of the APIs in this file are generic and can be used for Containers in # general, or e.g. Units (subclass of Container) in particular. These type diff --git a/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py index 1fd39a912..40c6e54bc 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py @@ -1,10 +1,12 @@ # Generated by Django 4.2.18 on 2025-03-28 02:21 +import uuid + +import django.db.models.deletion from django.conf import settings from django.db import migrations, models -import django.db.models.deletion + import openedx_learning.lib.validators -import uuid class Migration(migrations.Migration): diff --git a/openedx_learning/apps/authoring/publishing/models/__init__.py b/openedx_learning/apps/authoring/publishing/models/__init__.py index 324cf27d3..7e13c426a 100644 --- a/openedx_learning/apps/authoring/publishing/models/__init__.py +++ b/openedx_learning/apps/authoring/publishing/models/__init__.py @@ -14,10 +14,10 @@ """ from .container import Container, ContainerVersion -from .draft_log import Draft, DraftChangeLogRecord, DraftChangeLog, DraftSideEffect +from .draft_log import Draft, DraftChangeLog, DraftChangeLogRecord, DraftSideEffect from .entity_list import EntityList, EntityListRow from .learning_package import LearningPackage -from .publish_log import PublishLog, PublishLogRecord, Published +from .publish_log import Published, PublishLog, PublishLogRecord from .publishable_entity import ( PublishableContentModelRegistry, PublishableEntity, From 3aa83605231f8a91c8027e4f3715f204fdae0924 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 4 Apr 2025 00:47:51 -0400 Subject: [PATCH 15/29] fix: handle edge case around resetting to previous draft versions, add comments --- .../apps/authoring/publishing/api.py | 63 +++++++++++++------ 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 0c445a509..b79f37007 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -232,7 +232,6 @@ def create_publishable_entity_version( version.id, set_at=created, set_by=created_by, - create_transaction=False, ) return version @@ -464,7 +463,6 @@ def set_draft_version( /, set_at: datetime | None = None, set_by: int | None = None, # User.id - create_transaction: bool = True, ) -> None: """ The fallback for singledispatch is supposed to take a superclass of the @@ -482,7 +480,6 @@ def _( /, set_at: datetime | None = None, set_by: int | None = None, # User.id - create_transaction: bool = True, ) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. @@ -506,19 +503,24 @@ def _( if set_at is None: set_at = datetime.now(tz=timezone.utc) - tx_context = atomic() if create_transaction else nullcontext() + # If the Draft is already pointing at this version, there's nothing to do. + old_version_id = draft.version_id + if old_version_id == publishable_entity_version_pk: + return - with tx_context: - old_version_id = draft.version_id + with atomic(savepoint=False): + # The actual update of the Draft model is here. Everything after this + # block is bookkeeping in our DraftChangeLog. draft.version_id = publishable_entity_version_pk draft.save() - learning_package_id = draft.entity.learning_package_id - # 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. - active_change_log = DraftChangeLogContext.get_active_draft_change_log(learning_package_id) + learning_package_id = draft.entity.learning_package_id + active_change_log = DraftChangeLogContext.get_active_draft_change_log( + learning_package_id + ) change_log = active_change_log or DraftChangeLog.objects.create( learning_package_id=learning_package_id, changed_at=set_at, @@ -561,13 +563,11 @@ def _( /, set_at: datetime | None = None, set_by: int | None = None, # User.id - create_transaction: bool = True, ) -> None: """ Alias for set_draft_version taking PublishableEntity.id instead of a Draft. """ - tx_context = atomic() if create_transaction else nullcontext() - with tx_context: + with atomic(savepoint=False): draft, _created = Draft.objects.select_related("entity") \ .get_or_create(entity_id=publishable_entity_id) set_draft_version( @@ -575,7 +575,6 @@ def _( publishable_entity_version_pk, set_at=set_at, set_by=set_by, - create_transaction=False, ) @@ -602,16 +601,33 @@ def _add_to_existing_draft_change_log( and new_version = v3. """ try: - # If there is already a DraftChangeLogRecord for this PublishableEntity, - # we update the new_version_id. We keep the old_version_id value - # though, because that represents what it was before this - # DraftChangeLog, and we don't want to lose that information. + # Check to see if this PublishableEntity has already been changed in + # this DraftChangeLog. If so, we update that record instead of creating + # a new one. change = DraftChangeLogRecord.objects.get( draft_change_log=active_change_log, entity_id=entity_id, ) - change.new_version_id = new_version_id - change.save() + if change.old_version_id == new_version_id: + # 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. + # 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. + # + # It's important that we remove these cases, because we use the + # old_version == new_version convention to record entities that have + # changed purely due to side-effects. + change.delete() + 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 + # if we change the same entity multiple times in the DraftChangeLog. + change.new_version_id = new_version_id + change.save() except DraftChangeLogRecord.DoesNotExist: # If we're here, this is the first DraftChangeLogRecord we're making for # this PublishableEntity in the active DraftChangeLog. @@ -649,6 +665,13 @@ def _create_container_side_effects_for_draft_change( side-effect that a Component change affects a Unit if the Unit version is also changed in the same DraftChangeLog. + The `processed_entity_ids` set holds the entity IDs that we've already + calculated side-effects for. This is to save us from recalculating side- + effects for the same ancestor relationships over and over again. So if we're + calling this function in a loop for all the Components in a Unit, we won't + be recalculating the Unit's side-effect on its Subsection, and its + Subsection's side-effect on its Section. + TODO: This could get very expensive with the get_containers_with_entity calls. We should measure the impact of this. """ @@ -792,7 +815,7 @@ def reset_drafts_to_published( else: published_version_id = None - set_draft_version(draft, published_version_id, create_transaction=False) + set_draft_version(draft, published_version_id) def register_content_models( From 6c895ef8882ac48d311eb7cf9aafaf307ea0cafd Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 4 Apr 2025 17:03:37 -0400 Subject: [PATCH 16/29] temp: minor comment fixes --- openedx_learning/apps/authoring/publishing/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index b79f37007..705a073c5 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -663,7 +663,7 @@ def _create_container_side_effects_for_draft_change( This should only be run after the DraftChangeLogRecord has been otherwise fully written out. We want to avoid the scenario where we create a side-effect that a Component change affects a Unit if the Unit version is - also changed in the same DraftChangeLog. + also changed (maybe even deleted) in the same DraftChangeLog. The `processed_entity_ids` set holds the entity IDs that we've already calculated side-effects for. This is to save us from recalculating side- @@ -676,6 +676,7 @@ def _create_container_side_effects_for_draft_change( calls. We should measure the impact of this. """ if processed_entity_ids is None: + # An optimization, but also a guard against infinite side-effect loops. processed_entity_ids = set() changes_and_containers = [ From 141a4fb5a670c4d00c0864c29e2e42a7de4b48f6 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 9 Apr 2025 13:37:34 -0400 Subject: [PATCH 17/29] test: add tests --- .../apps/authoring/publishing/api.py | 67 +-- .../authoring/publishing/contextmanagers.py | 7 + .../apps/authoring/publishing/test_api.py | 464 ++++++++++++++---- 3 files changed, 404 insertions(+), 134 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 705a073c5..d87403248 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -456,26 +456,8 @@ def get_published_version(publishable_entity_id: int, /) -> PublishableEntityVer return published.version -@singledispatch def set_draft_version( - arg: object, - publishable_entity_version_pk: int | None, - /, - set_at: datetime | None = None, - set_by: int | None = None, # User.id -) -> None: - """ - The fallback for singledispatch is supposed to take a superclass of the - different registered versions (Draft and int in this case). This is that - fallback, but the "real" implementation is the Draft-registered version - below. - """ - raise NotImplementedError() - - -@set_draft_version.register(Draft) -def _( - draft: Draft, + draft_or_id: Draft | int, publishable_entity_version_pk: int | None, /, set_at: datetime | None = None, @@ -484,6 +466,10 @@ def _( """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. + The ``draft`` argument can be either a Draft model object, or the primary + key of a Draft/PublishableEntity (Draft is defined so these will be the same + value). + This would most commonly be used to set the Draft to point to a newly created PublishableEntityVersion that was created in Studio (because someone edited some content). Setting a Draft's version to None is like deleting it @@ -503,12 +489,23 @@ def _( if set_at is None: set_at = datetime.now(tz=timezone.utc) - # If the Draft is already pointing at this version, there's nothing to do. - old_version_id = draft.version_id - if old_version_id == publishable_entity_version_pk: - return - with atomic(savepoint=False): + if isinstance(draft_or_id, Draft): + draft = draft_or_id + elif isinstance(draft_or_id, int): + draft, _created = Draft.objects.select_related("entity") \ + .get_or_create(entity_id=draft_or_id) + else: + class_name = draft_or_id.__class__.__name__ + raise TypeError( + f"draft_or_id must be a Draft or int, not ({class_name})" + ) + + # If the Draft is already pointing at this version, there's nothing to do. + old_version_id = draft.version_id + if old_version_id == publishable_entity_version_pk: + return + # The actual update of the Draft model is here. Everything after this # block is bookkeeping in our DraftChangeLog. draft.version_id = publishable_entity_version_pk @@ -556,28 +553,6 @@ def _( _create_container_side_effects_for_draft_change(change) -@set_draft_version.register(int) -def _( - publishable_entity_id: int, - publishable_entity_version_pk: int | None, - /, - set_at: datetime | None = None, - set_by: int | None = None, # User.id -) -> None: - """ - Alias for set_draft_version taking PublishableEntity.id instead of a Draft. - """ - with atomic(savepoint=False): - draft, _created = Draft.objects.select_related("entity") \ - .get_or_create(entity_id=publishable_entity_id) - set_draft_version( - draft, - publishable_entity_version_pk, - set_at=set_at, - set_by=set_by, - ) - - def _add_to_existing_draft_change_log( active_change_log: DraftChangeLog, entity_id: int, diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py index 19b9d048b..c57941c39 100644 --- a/openedx_learning/apps/authoring/publishing/contextmanagers.py +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -136,6 +136,13 @@ def __exit__(self, exc_type, exc_value, traceback): for exit_callback in self.exit_callbacks: exit_callback(draft_change_log) + # Edge case: the draft changes that accumulated during our context + # cancel each other out, and there are no actual + # DraftChangeLogRecords at the end. In this case, we might as well + # delete the entire DraftChangeLog. + if not draft_change_log.records.exists(): + draft_change_log.delete() + self._draft_change_logs.set(draft_change_logs) return super().__exit__(exc_type, exc_value, traceback) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 75aebde1c..1f24d82e0 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -13,7 +13,9 @@ from openedx_learning.apps.authoring.publishing.models import ( Container, ContainerVersion, + Draft, DraftChangeLog, + DraftChangeLogRecord, DraftSideEffect, LearningPackage, PublishableEntity, @@ -136,94 +138,6 @@ def setUpTestData(cls) -> None: created=cls.now, ) - def test_simple_draft_changeset(self) -> None: - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): - 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 DraftChangeSet... - entity3 = publishing_api.create_publishable_entity( - self.learning_package_1.id, - "my_entity3", - created=self.now, - created_by=None, - ) - publishing_api.create_publishable_entity_version( - entity3.id, - version_num=1, - title="An Entity 🌴 3", - created=self.now, - created_by=None, - ) - draft_sets = list(DraftChangeLog.objects.all().order_by('id')) - assert len(draft_sets) == 2 - assert len(draft_sets[1].records.all()) == 1 - - def test_nested_draft_changesets(self) -> None: - pass - - 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): - 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 - changes = list(draft_sets[0].records.all()) - assert len(changes) == 1 - change = changes[0] - assert change.old_version is None - assert change.new_version is not None - assert change.new_version.version_num == 2 - def test_draft_lifecycle(self) -> None: """ Test basic lifecycle of a Draft. @@ -253,6 +167,36 @@ def test_draft_lifecycle(self) -> None: deleted_entity_version = publishing_api.get_draft_version(entity.id) assert deleted_entity_version is None + def test_set_draft_args(self) -> None: + """Make sure it works with Draft and int, and raises exception otherwise""" + entity = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity", + created=self.now, + created_by=None, + ) + entity_version = publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="An Entity 🌴", + created=self.now, + created_by=None, + ) + + # Int calling version + publishing_api.soft_delete_draft(entity_version.id) + publishing_api.set_draft_version(entity.draft.pk, entity_version.pk) + assert Draft.objects.get(entity=entity).version == entity_version + + # Draft calling version + publishing_api.soft_delete_draft(entity_version.id) + publishing_api.set_draft_version(entity.draft, entity_version.pk) + assert Draft.objects.get(entity=entity).version == entity_version + + # Unrecognized type + with pytest.raises(TypeError): + publishing_api.set_draft_version(1.0, entity_version.pk) + def test_soft_deletes(self) -> None: """Test the publishing behavior of soft deletes.""" entity = publishing_api.create_publishable_entity( @@ -598,6 +542,296 @@ def _get_draft_version_num(self, entity: PublishableEntity) -> int | None: return None +class DraftChangeLogTestCase(TestCase): + """ + Test basic operations with DraftChangeLogs and bulk draft operations. + """ + now: datetime + learning_package_1: LearningPackage + learning_package_2: LearningPackage + + @classmethod + def setUpTestData(cls) -> None: + cls.now = datetime(2024, 1, 28, 16, 45, 30, tzinfo=timezone.utc) + cls.learning_package_1 = publishing_api.create_learning_package( + "my_package_key_1", + "Draft Testing LearningPackage 🔥 1", + created=cls.now, + ) + cls.learning_package_2 = publishing_api.create_learning_package( + "my_package_key_2", + "Draft Testing LearningPackage 🔥 2", + created=cls.now, + ) + + 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): + 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, + ) + draft_sets = list(DraftChangeLog.objects.all().order_by('id')) + assert len(draft_sets) == 2 + assert len(draft_sets[1].records.all()) == 1 + + # Now make one entirely redundant change, and make sure it didn't create + # anything (setting a draft to the same version it already was should be + # a no-op). + publishing_api.set_draft_version(entity3.id, e3_v1.pk) + draft_sets = list(DraftChangeLog.objects.all().order_by('id')) + assert len(draft_sets) == 2 + assert len(draft_sets[1].records.all()) == 1 + + 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: + 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: + # 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 + lp1_e1_v2 = publishing_api.create_publishable_entity_version( + 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 + # didn't make a new one (or go to the inner context). + assert DraftChangeLog.objects.all().count() == 2 + assert DraftChangeLogRecord.objects.all().count() == 1 + lp1_e1_record = DraftChangeLogRecord.objects.first() + assert lp1_e1_record is not None + assert lp1_e1_record.old_version is None + assert lp1_e1_record.new_version == lp1_e1_v2 + assert lp1_e1_record.draft_change_log.learning_package == self.learning_package_1 + + # This will go to the inner context: + 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 + lp2_e1_v2 = publishing_api.create_publishable_entity_version( + 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. + assert dcl_1.records.count() == 1 + + # Check the state of the second/inner DraftChangeLog + assert dcl_2.records.count() == 1 + lp2_e1_record = dcl_2.records.get(entity=lp2_e1) + assert lp2_e1_record.old_version is None + assert lp2_e1_record.new_version == lp2_e1_v1 + + # We should have 3 DraftChangeLogs, because the last change to lp2_e1 + # was done outside of any context for Learning Package 2. Instead of + # using Learning Package 1's context, it should create its own + # DraftChangeLog: + assert DraftChangeLog.objects.count() == 3 + implicit_dcl = DraftChangeLog.objects.order_by('id').last() + assert implicit_dcl is not None + assert implicit_dcl.records.count() == 1 + implicit_lp2_e1_record = implicit_dcl.records.get(entity=lp2_e1) + assert implicit_lp2_e1_record.old_version == lp2_e1_v1 + assert implicit_lp2_e1_record.new_version == lp2_e1_v2 + + 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): + 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 + changes = list(draft_sets[0].records.all()) + assert len(changes) == 1 + change = changes[0] + assert change.old_version is None + assert change.new_version is not None + assert change.new_version.version_num == 2 + + 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): + # 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) + + # The change to entity_2 will persist + 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() + assert change_log is not None + assert change_log.records.count() == 1 + change = change_log.records.get(entity_id=entity_2.id) + assert change.old_version is None + assert change.new_version == e2_v1 + + 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): + 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) + + assert not DraftChangeLog.objects.all().exists() + + # This next call implicitly makes a DraftChangeLog + publishing_api.set_draft_version(entity.id, v1.pk) + 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): + 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) + + assert DraftChangeLog.objects.all().count() == 1 + + class ContainerTestCase(TestCase): """ Test basic operations with Drafts. @@ -781,3 +1015,57 @@ def test_bulk_parent_child_side_effects(self) -> None: def test_multiple_layers_of_containers(self): """Test stacking containers three layers deep.""" + # Note that these aren't real "components" and "units". Everything being + # tested is confined to the publishing app, so those concepts shouldn't + # be imported here. They're just named this way to make it more obvious + # what the intended hierarchy is for testing container nesting. + component = publishing_api.create_publishable_entity( + self.learning_package.id, "component_1", created=self.now, created_by=None, + ) + component_v1 = publishing_api.create_publishable_entity_version( + component.id, version_num=1, title="Component 1 🌴", created=self.now, created_by=None, + ) + unit = publishing_api.create_container( + self.learning_package.id, "unit_1", created=self.now, created_by=None, + ) + unit_v1: ContainerVersion = publishing_api.create_container_version( + unit.pk, + 1, + title="My Unit", + publishable_entities_pks=[component.pk], + entity_version_pks=None, + created=self.now, + created_by=None, + ) + subsection = publishing_api.create_container( + self.learning_package.id, "subsection_1", created=self.now, created_by=None, + ) + subsection_v1: ContainerVersion = publishing_api.create_container_version( + subsection.pk, + 1, + title="My Subsection", + publishable_entities_pks=[unit.pk], + entity_version_pks=None, + created=self.now, + created_by=None, + ) + + # At this point, no side-effects exist yet because we built it from the + # 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: + publishing_api.create_publishable_entity_version( + component.id, version_num=2, title="Component 1v2🌴", created=self.now, created_by=None, + ) + + assert DraftSideEffect.objects.count() == 2 + component_change = change_log.records.get(entity=component) + unit_change = change_log.records.get(entity=unit.publishable_entity) + subsection_change = change_log.records.get(entity=subsection.publishable_entity) + + assert not component_change.affected_by.exists() + assert unit_change.affected_by.count() == 1 + assert unit_change.affected_by.first().cause == component_change + assert subsection_change.affected_by.count() == 1 + assert subsection_change.affected_by.first().cause == unit_change From 3b8a2734567e95ddec5b99abca2d4c2fa863ac94 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 9 Apr 2025 13:43:42 -0400 Subject: [PATCH 18/29] temp: linter fixups --- openedx_learning/apps/authoring/publishing/api.py | 1 - .../apps/authoring/publishing/test_api.py | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index d87403248..f17c37366 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -10,7 +10,6 @@ from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum -from functools import singledispatch from typing import ContextManager, TypeVar from django.core.exceptions import ObjectDoesNotExist, ValidationError diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 1f24d82e0..19a0f02c1 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -195,7 +195,7 @@ def test_set_draft_args(self) -> None: # Unrecognized type with pytest.raises(TypeError): - publishing_api.set_draft_version(1.0, entity_version.pk) + publishing_api.set_draft_version(1.0, entity_version.pk) # type: ignore[arg-type] def test_soft_deletes(self) -> None: """Test the publishing behavior of soft deletes.""" @@ -1022,13 +1022,13 @@ def test_multiple_layers_of_containers(self): component = publishing_api.create_publishable_entity( self.learning_package.id, "component_1", created=self.now, created_by=None, ) - component_v1 = publishing_api.create_publishable_entity_version( + publishing_api.create_publishable_entity_version( component.id, version_num=1, title="Component 1 🌴", created=self.now, created_by=None, ) unit = publishing_api.create_container( self.learning_package.id, "unit_1", created=self.now, created_by=None, ) - unit_v1: ContainerVersion = publishing_api.create_container_version( + publishing_api.create_container_version( unit.pk, 1, title="My Unit", @@ -1040,7 +1040,7 @@ def test_multiple_layers_of_containers(self): subsection = publishing_api.create_container( self.learning_package.id, "subsection_1", created=self.now, created_by=None, ) - subsection_v1: ContainerVersion = publishing_api.create_container_version( + publishing_api.create_container_version( subsection.pk, 1, title="My Subsection", From fce7154457a23f8f67617f0df9c41699de03a6e1 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 9 Apr 2025 14:53:38 -0400 Subject: [PATCH 19/29] temp: add more type annotations --- openedx_learning/apps/authoring/publishing/api.py | 6 +++++- .../apps/authoring/publishing/contextmanagers.py | 11 +++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index f17c37366..e59382939 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -1416,7 +1416,11 @@ def get_container_children_count( return container_version.entity_list.entitylistrow_set.filter(**filter_deleted).count() -def bulk_draft_changes_for(learning_package_id: int, changed_by=None, changed_at=None) -> DraftChangeLogContext: +def bulk_draft_changes_for( + learning_package_id: int, + changed_by: int | None = None, + changed_at: datetime | None = None +) -> DraftChangeLogContext: """ Context manager to do a single batch of Draft changes in. """ diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py index c57941c39..9c81fadc4 100644 --- a/openedx_learning/apps/authoring/publishing/contextmanagers.py +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -6,6 +6,7 @@ """ from contextvars import ContextVar from datetime import datetime, timezone +from typing import Callable from django.db.transaction import Atomic @@ -50,7 +51,13 @@ class to essentially keep the active DraftChangeChangeLog in a global """ _draft_change_logs: ContextVar[list | None] = ContextVar('_draft_change_logs', default=None) - def __init__(self, learning_package_id, changed_at=None, changed_by=None, exit_callbacks=None): + def __init__( + self, + learning_package_id: int, + changed_at: datetime | None = None, + changed_by: int | None = None, + exit_callbacks: list[Callable] | None = None + ) -> None: super().__init__(using=None, savepoint=False, durable=False) self.learning_package_id = learning_package_id @@ -111,7 +118,7 @@ def __enter__(self): self.draft_change_log = DraftChangeLog.objects.create( learning_package_id=self.learning_package_id, - changed_by=self.changed_by, + changed_by_id=self.changed_by, changed_at=self.changed_at, ) draft_change_sets = self._draft_change_logs.get() From 951afb60870f92da0c3ba6797b91a03cd25f1dac Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 9 Apr 2025 17:19:56 -0400 Subject: [PATCH 20/29] fix: broken test was passing in entity version ID isntead of entity ID --- .../openedx_learning/apps/authoring/publishing/test_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 19a0f02c1..111ebc639 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -171,7 +171,7 @@ def test_set_draft_args(self) -> None: """Make sure it works with Draft and int, and raises exception otherwise""" entity = publishing_api.create_publishable_entity( self.learning_package_1.id, - "my_entity", + "my_set_draft_args_entity", created=self.now, created_by=None, ) @@ -184,12 +184,12 @@ def test_set_draft_args(self) -> None: ) # Int calling version - publishing_api.soft_delete_draft(entity_version.id) + publishing_api.soft_delete_draft(entity.id) publishing_api.set_draft_version(entity.draft.pk, entity_version.pk) assert Draft.objects.get(entity=entity).version == entity_version # Draft calling version - publishing_api.soft_delete_draft(entity_version.id) + publishing_api.soft_delete_draft(entity.id) publishing_api.set_draft_version(entity.draft, entity_version.pk) assert Draft.objects.get(entity=entity).version == entity_version From 643758d0c14028dd0d18344b283188aa2f41f97b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 11 Apr 2025 14:47:23 -0400 Subject: [PATCH 21/29] refactor: update for rebase-introduced migration and API change --- .../migrations/0005_draftchangelog.py | 68 -------------- .../0006_bootstrap_draftchangelog.py | 94 ------------------- .../apps/authoring/publishing/test_api.py | 22 +++-- .../apps/authoring/units/test_api.py | 2 +- 4 files changed, 15 insertions(+), 171 deletions(-) delete mode 100644 openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py delete mode 100644 openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py diff --git a/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py deleted file mode 100644 index 40c6e54bc..000000000 --- a/openedx_learning/apps/authoring/publishing/migrations/0005_draftchangelog.py +++ /dev/null @@ -1,68 +0,0 @@ -# Generated by Django 4.2.18 on 2025-03-28 02:21 - -import uuid - -import django.db.models.deletion -from django.conf import settings -from django.db import migrations, models - -import openedx_learning.lib.validators - - -class Migration(migrations.Migration): - - dependencies = [ - migrations.swappable_dependency(settings.AUTH_USER_MODEL), - ('oel_publishing', '0004_publishableentity_can_stand_alone'), - ] - - operations = [ - migrations.CreateModel( - name='DraftChangeLog', - fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), - ('changed_at', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), - ('changed_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), - ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), - ], - options={ - 'verbose_name': 'Draft Change Log', - 'verbose_name_plural': 'Draft Change Logs', - }, - ), - migrations.CreateModel( - name='DraftChangeLogRecord', - fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('draft_change_log', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='changes', to='oel_publishing.draftchangelog')), - ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), - ('new_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentityversion')), - ('old_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')), - ], - options={ - 'verbose_name': 'Draft Log', - 'verbose_name_plural': 'Draft Log', - }, - ), - migrations.CreateModel( - name='DraftSideEffect', - fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('cause', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='causes', to='oel_publishing.draftchangelogrecord')), - ('effect', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='caused_by', to='oel_publishing.draftchangelogrecord')), - ], - ), - migrations.AddConstraint( - model_name='draftsideeffect', - constraint=models.UniqueConstraint(fields=('cause', 'effect'), name='oel_pub_dse_uniq_c_e'), - ), - migrations.AddIndex( - model_name='draftchangelogrecord', - index=models.Index(fields=['entity', '-draft_change_log'], name='oel_dlr_idx_entity_rdcl'), - ), - migrations.AddConstraint( - model_name='draftchangelogrecord', - constraint=models.UniqueConstraint(fields=('draft_change_log', 'entity'), name='oel_dlr_uniq_dcl'), - ), - ] diff --git a/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py deleted file mode 100644 index 42c40eb36..000000000 --- a/openedx_learning/apps/authoring/publishing/migrations/0006_bootstrap_draftchangelog.py +++ /dev/null @@ -1,94 +0,0 @@ -""" -Bootstrap DraftChangeLogs - -DraftChangeLog and DraftChangeLogRecord are being introduced after Drafts, so -we're going to retroactively make entries for all the changes that were in our -Learning Packages. - -This migration will try to reconstruct create, edit, reset-to-published, and -delete operations, but it won't be fully accurate because we only have the -create dates of the versions and the current state of active Drafts to go on. -This means we won't accurately capture when things were deleted and then reset, -or when reset and then later edited. We're also missing the user for a number of -these operations, so we'll add those with null created_by entries. Addressing -these gaps is a big part of why we created DraftChangeLogs in the first place. -""" -# Generated by Django 4.2.18 on 2025-03-13 10:29 -import logging -from datetime import datetime, timezone - -from django.db import migrations - -logger = logging.getLogger(__name__) - - -def bootstrap_draft_change_logs(apps, schema_editor): - """ - Create a fake DraftChangeSet that encompasses the state of current Drafts. - """ - LearningPackage = apps.get_model("oel_publishing", "LearningPackage") - PublishableEntityVersion = apps.get_model("oel_publishing", "PublishableEntityVersion") - - Draft = apps.get_model("oel_publishing", "Draft") - DraftChangeLogRecord = apps.get_model("oel_publishing", "DraftChangeLogRecord") - DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") - now = datetime.now(tz=timezone.utc) - - for learning_package in LearningPackage.objects.all().order_by("key"): - logger.info(f"Creating bootstrap DraftChangeLogs for {learning_package.key}") - pub_ent_versions = PublishableEntityVersion.objects.filter( - entity__learning_package=learning_package - ).select_related("entity") - - # First cycle though all the simple create/edit operations... - last_version_seen = {} # PublishableEntity.id -> PublishableEntityVersion.id - for pub_ent_version in pub_ent_versions.order_by("pk"): - draft_change_log = DraftChangeLog.objects.create( - learning_package=learning_package, - changed_at=pub_ent_version.created, - changed_by=pub_ent_version.created_by, - ) - DraftChangeLogRecord.objects.create( - draft_change_log=draft_change_log, - entity=pub_ent_version.entity, - old_version_id=last_version_seen.get(pub_ent_version.entity.id), - new_version_id=pub_ent_version.id, - ) - last_version_seen[pub_ent_version.entity.id] = pub_ent_version.id - - # Now that we've created change sets for create/edit operations, we look - # at the latest state of the Draft model in order to determine whether - # we need to apply deletes or resets. - for draft in Draft.objects.filter(entity__learning_package=learning_package).order_by("entity_id"): - last_version_id = last_version_seen.get(draft.entity_id) - if draft.version_id == last_version_id: - continue - # We don't really know who did this or when, so we use None and now. - draft_change_log = DraftChangeLog.objects.create( - learning_package=learning_package, - changed_at=now, - changed_by=None, - ) - DraftChangeLogRecord.objects.create( - draft_change_log=draft_change_log, - entity_id=draft.entity_id, - old_version_id=last_version_id, - new_version_id=draft.version_id, - ) - - -def delete_draft_change_logs(apps, schema_editor): - logger.info(f"Deleting all DraftChangeLogs (reverse migration)") - DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") - DraftChangeLog.objects.all().delete() - - -class Migration(migrations.Migration): - - dependencies = [ - ('oel_publishing', '0005_draftchangelog'), - ] - - operations = [ - migrations.RunPython(bootstrap_draft_change_logs, reverse_code=delete_draft_change_logs) - ] diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 111ebc639..b9e45ec91 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -886,8 +886,10 @@ def test_parent_child_side_effects(self) -> None: container.pk, 1, title="My Container", - publishable_entities_pks=[child_1.id, child_2.id], - entity_version_pks=None, + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=child_1.pk), + publishing_api.ContainerEntityRow(entity_pk=child_2.pk), + ], created=self.now, created_by=None, ) @@ -965,8 +967,10 @@ def test_bulk_parent_child_side_effects(self) -> None: container.pk, 1, title="My Container", - publishable_entities_pks=[child_1.id, child_2.id], - entity_version_pks=None, + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=child_1.pk), + publishing_api.ContainerEntityRow(entity_pk=child_2.pk), + ], created=self.now, created_by=None, ) @@ -1032,8 +1036,9 @@ def test_multiple_layers_of_containers(self): unit.pk, 1, title="My Unit", - publishable_entities_pks=[component.pk], - entity_version_pks=None, + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=component.pk), + ], created=self.now, created_by=None, ) @@ -1044,8 +1049,9 @@ def test_multiple_layers_of_containers(self): subsection.pk, 1, title="My Subsection", - publishable_entities_pks=[unit.pk], - entity_version_pks=None, + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=unit.pk), + ], created=self.now, created_by=None, ) diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index 9d0ca8873..1e7c0ea14 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -173,7 +173,7 @@ def test_create_unit_queries(self): # The exact numbers here aren't too important - this is just to alert us if anything significant changes. with self.assertNumQueries(25): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(28): + with self.assertNumQueries(29): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2") From 7b65c811a47d46703e98185e4ffb7f2070dfbc13 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 11 Apr 2025 14:48:17 -0400 Subject: [PATCH 22/29] refactor: no, really, changing the migrations now --- .../migrations/0006_draftchangelog.py | 68 ++++++++++++++ .../0007_bootstrap_draftchangelog.py | 94 +++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 openedx_learning/apps/authoring/publishing/migrations/0006_draftchangelog.py create mode 100644 openedx_learning/apps/authoring/publishing/migrations/0007_bootstrap_draftchangelog.py diff --git a/openedx_learning/apps/authoring/publishing/migrations/0006_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0006_draftchangelog.py new file mode 100644 index 000000000..b72e16843 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0006_draftchangelog.py @@ -0,0 +1,68 @@ +# Generated by Django 4.2.18 on 2025-03-28 02:21 + +import uuid + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + +import openedx_learning.lib.validators + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('oel_publishing', '0005_alter_entitylistrow_options'), + ] + + operations = [ + migrations.CreateModel( + name='DraftChangeLog', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('changed_at', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), + ('changed_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), + ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), + ], + options={ + 'verbose_name': 'Draft Change Log', + 'verbose_name_plural': 'Draft Change Logs', + }, + ), + migrations.CreateModel( + name='DraftChangeLogRecord', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('draft_change_log', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='changes', to='oel_publishing.draftchangelog')), + ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), + ('new_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentityversion')), + ('old_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')), + ], + options={ + 'verbose_name': 'Draft Log', + 'verbose_name_plural': 'Draft Log', + }, + ), + migrations.CreateModel( + name='DraftSideEffect', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('cause', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='causes', to='oel_publishing.draftchangelogrecord')), + ('effect', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='caused_by', to='oel_publishing.draftchangelogrecord')), + ], + ), + migrations.AddConstraint( + model_name='draftsideeffect', + constraint=models.UniqueConstraint(fields=('cause', 'effect'), name='oel_pub_dse_uniq_c_e'), + ), + migrations.AddIndex( + model_name='draftchangelogrecord', + index=models.Index(fields=['entity', '-draft_change_log'], name='oel_dlr_idx_entity_rdcl'), + ), + migrations.AddConstraint( + model_name='draftchangelogrecord', + constraint=models.UniqueConstraint(fields=('draft_change_log', 'entity'), name='oel_dlr_uniq_dcl'), + ), + ] diff --git a/openedx_learning/apps/authoring/publishing/migrations/0007_bootstrap_draftchangelog.py b/openedx_learning/apps/authoring/publishing/migrations/0007_bootstrap_draftchangelog.py new file mode 100644 index 000000000..5c8ab3735 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0007_bootstrap_draftchangelog.py @@ -0,0 +1,94 @@ +""" +Bootstrap DraftChangeLogs + +DraftChangeLog and DraftChangeLogRecord are being introduced after Drafts, so +we're going to retroactively make entries for all the changes that were in our +Learning Packages. + +This migration will try to reconstruct create, edit, reset-to-published, and +delete operations, but it won't be fully accurate because we only have the +create dates of the versions and the current state of active Drafts to go on. +This means we won't accurately capture when things were deleted and then reset, +or when reset and then later edited. We're also missing the user for a number of +these operations, so we'll add those with null created_by entries. Addressing +these gaps is a big part of why we created DraftChangeLogs in the first place. +""" +# Generated by Django 4.2.18 on 2025-03-13 10:29 +import logging +from datetime import datetime, timezone + +from django.db import migrations + +logger = logging.getLogger(__name__) + + +def bootstrap_draft_change_logs(apps, schema_editor): + """ + Create a fake DraftChangeSet that encompasses the state of current Drafts. + """ + LearningPackage = apps.get_model("oel_publishing", "LearningPackage") + PublishableEntityVersion = apps.get_model("oel_publishing", "PublishableEntityVersion") + + Draft = apps.get_model("oel_publishing", "Draft") + DraftChangeLogRecord = apps.get_model("oel_publishing", "DraftChangeLogRecord") + DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") + now = datetime.now(tz=timezone.utc) + + for learning_package in LearningPackage.objects.all().order_by("key"): + logger.info(f"Creating bootstrap DraftChangeLogs for {learning_package.key}") + pub_ent_versions = PublishableEntityVersion.objects.filter( + entity__learning_package=learning_package + ).select_related("entity") + + # First cycle though all the simple create/edit operations... + last_version_seen = {} # PublishableEntity.id -> PublishableEntityVersion.id + for pub_ent_version in pub_ent_versions.order_by("pk"): + draft_change_log = DraftChangeLog.objects.create( + learning_package=learning_package, + changed_at=pub_ent_version.created, + changed_by=pub_ent_version.created_by, + ) + DraftChangeLogRecord.objects.create( + draft_change_log=draft_change_log, + entity=pub_ent_version.entity, + old_version_id=last_version_seen.get(pub_ent_version.entity.id), + new_version_id=pub_ent_version.id, + ) + last_version_seen[pub_ent_version.entity.id] = pub_ent_version.id + + # Now that we've created change sets for create/edit operations, we look + # at the latest state of the Draft model in order to determine whether + # we need to apply deletes or resets. + for draft in Draft.objects.filter(entity__learning_package=learning_package).order_by("entity_id"): + last_version_id = last_version_seen.get(draft.entity_id) + if draft.version_id == last_version_id: + continue + # We don't really know who did this or when, so we use None and now. + draft_change_log = DraftChangeLog.objects.create( + learning_package=learning_package, + changed_at=now, + changed_by=None, + ) + DraftChangeLogRecord.objects.create( + draft_change_log=draft_change_log, + entity_id=draft.entity_id, + old_version_id=last_version_id, + new_version_id=draft.version_id, + ) + + +def delete_draft_change_logs(apps, schema_editor): + logger.info(f"Deleting all DraftChangeLogs (reverse migration)") + DraftChangeLog = apps.get_model("oel_publishing", "DraftChangeLog") + DraftChangeLog.objects.all().delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0006_draftchangelog'), + ] + + operations = [ + migrations.RunPython(bootstrap_draft_change_logs, reverse_code=delete_draft_change_logs) + ] From 7063f5de2fbd2cc3b34f6a6818f66df7aaa0cb03 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 11 Apr 2025 18:55:31 -0400 Subject: [PATCH 23/29] chore: no_pii annotations on draft models --- .../apps/authoring/publishing/models/draft_log.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/openedx_learning/apps/authoring/publishing/models/draft_log.py b/openedx_learning/apps/authoring/publishing/models/draft_log.py index 2daf6f6c1..6e85a6ec8 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -38,6 +38,8 @@ class Draft(models.Model): separation of lifecycle events: i.e. this table *only* changes when drafts are updated, not when publishing happens. The Published model only changes when something is published. + + .. no_pii """ # If we're removing a PublishableEntity entirely, also remove the Draft # entry for it. This isn't a normal operation, but can happen if you're @@ -69,6 +71,8 @@ class DraftChangeLog(models.Model): PublishableEntity, we will get multiple DraftChangeLogRecords if changing that entity causes side-effects. See the docstrings for DraftChangeLogRecord and DraftSideEffect for more details. + + .. no_pii: """ uuid = immutable_uuid_field() learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) @@ -178,6 +182,8 @@ class DraftChangeLogRecord(models.Model): represent that in a useful way because that Unit *is* a part of the change set represented by a DraftChangeLog, even if its own versioned data hasn't changed. + + .. no_pii: """ draft_change_log = models.ForeignKey( DraftChangeLog, @@ -282,6 +288,8 @@ class DraftSideEffect(models.Model): current draft of U1 (U1.v2). A DraftChangeLog is considered a single atomic operation, so there was never a point at which C1.v1 -> C1.v2 affected the draft state of U1. + + .. no_pii: """ cause = models.ForeignKey( DraftChangeLogRecord, From 26cbd1effb9119851d8c222e6d812ea1051acfb6 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 15 Apr 2025 15:37:47 -0400 Subject: [PATCH 24/29] fix: enrich type annotation of exit_callbacks --- openedx_learning/apps/authoring/publishing/contextmanagers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/contextmanagers.py b/openedx_learning/apps/authoring/publishing/contextmanagers.py index 9c81fadc4..64ca9fb1f 100644 --- a/openedx_learning/apps/authoring/publishing/contextmanagers.py +++ b/openedx_learning/apps/authoring/publishing/contextmanagers.py @@ -4,6 +4,8 @@ Do not use this directly outside the publishing app. Use the public API's bulk_draft_changes_for instead (which will invoke this internally). """ +from __future__ import annotations + from contextvars import ContextVar from datetime import datetime, timezone from typing import Callable @@ -56,7 +58,7 @@ def __init__( learning_package_id: int, changed_at: datetime | None = None, changed_by: int | None = None, - exit_callbacks: list[Callable] | None = None + exit_callbacks: list[Callable[[DraftChangeLog], None]] | None = None ) -> None: super().__init__(using=None, savepoint=False, durable=False) From a4edf0d5c118d0f209d5e8ac361a1fbd631894ca Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 16 Apr 2025 13:40:55 -0400 Subject: [PATCH 25/29] docs: get_containers_with_entity TODO followup link --- openedx_learning/apps/authoring/publishing/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index e59382939..cf555dfb7 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -1371,7 +1371,7 @@ def get_containers_with_entity( ignore_pinned: if true, ignore any pinned references to the entity. """ if ignore_pinned: - # TODO: Do we need to run distinct() on this? + # TODO: Do we need to run distinct() on this? Will fix in https://github.com/openedx/openedx-learning/issues/303 qs = Container.objects.filter( # Note: these two conditions must be in the same filter() call, or the query won't be correct. publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 From b0f911a9644bdc133054b6c42309c5e49d6eb497 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 16 Apr 2025 14:36:51 -0400 Subject: [PATCH 26/29] refactor: simplify active_change_log conditional --- openedx_learning/apps/authoring/publishing/api.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index cf555dfb7..1fd992ed5 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -517,12 +517,6 @@ def set_draft_version( active_change_log = DraftChangeLogContext.get_active_draft_change_log( learning_package_id ) - change_log = active_change_log or DraftChangeLog.objects.create( - learning_package_id=learning_package_id, - changed_at=set_at, - changed_by_id=set_by, - ) - if active_change_log: change = _add_to_existing_draft_change_log( active_change_log, @@ -543,6 +537,11 @@ def set_draft_version( # and add our DraftChangeLogRecord to it. This has the 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, + ) change = DraftChangeLogRecord.objects.create( draft_change_log=change_log, entity_id=draft.entity_id, From c7aa0a25f2cebf632f36b1e3bd56f0a0e53f4d90 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 16 Apr 2025 14:47:14 -0400 Subject: [PATCH 27/29] docs: rm outdated comment --- openedx_learning/apps/authoring/publishing/api.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 1fd992ed5..338048ae4 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -738,9 +738,6 @@ 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. - - Also, there is no current immutable record for when a reset happens. It's - not like a publish that leaves an entry in the ``PublishLog``. """ if reset_at is None: reset_at = datetime.now(tz=timezone.utc) From bbc4dc264b8f475638ccdab557fa86497a7b5bf0 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 16 Apr 2025 15:10:32 -0400 Subject: [PATCH 28/29] docs: document bulk_draft_changes_for --- .../apps/authoring/publishing/api.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 338048ae4..4e1af6414 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -1419,6 +1419,32 @@ def bulk_draft_changes_for( ) -> DraftChangeLogContext: """ Context manager to do a single batch of Draft changes in. + + Each publishable entity that is edited in this context will be tied to a + single DraftChangeLogRecord, representing the cumulative changes made to + that entity. Upon closing of the context, side effects of these changes will + be calcuated, which may result in more DraftChangeLogRecords being created + or updated. The resulting DraftChangeLogRecords and DraftChangeSideEffects + will be tied together into a single DraftChangeLog, representing the + collective changes to the learning package that happened in this context. + All changes will be committed in a single atomic transaction. + + Example:: + + with bulk_draft_changes_for(learning_package.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) """ return DraftChangeLogContext( learning_package_id, From 36642eae3454194604c82911eae46d959c664547 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 16 Apr 2025 15:22:37 -0400 Subject: [PATCH 29/29] build: increment version, 0.22 -> 0.23 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index c15af56f2..d2a9779dd 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.22.0" +__version__ = "0.23.0"