diff --git a/openedx_learning/apps/authoring/publishing/admin.py b/openedx_learning/apps/authoring/publishing/admin.py index 7b144fa8c..56657a119 100644 --- a/openedx_learning/apps/authoring/publishing/admin.py +++ b/openedx_learning/apps/authoring/publishing/admin.py @@ -97,6 +97,7 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): list_display = [ "key", "draft_version", + "draft_state", "published_version", "uuid", "learning_package", @@ -110,6 +111,7 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): fields = [ "key", "draft_version", + "draft_state", "published_version", "uuid", "learning_package", @@ -121,6 +123,7 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): readonly_fields = [ "key", "draft_version", + "draft_state", "published_version", "uuid", "learning_package", @@ -145,6 +148,20 @@ def get_queryset(self, request): def see_also(self, entity): return one_to_one_related_model_html(entity) + def draft_version(self, entity): + if entity.draft.version: + return entity.draft.version.version_num + return None + + def draft_state(self, entity): + if entity.draft: + return entity.draft.state_hash + + def published_version(self, entity): + if entity.published.version: + return entity.published.version.version_num + return None + @admin.register(Published) class PublishedAdmin(ReadOnlyModelAdmin): diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 159b8aec0..83797d8ff 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -15,6 +15,7 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet from django.db.transaction import atomic +from openedx_learning.lib.fields import create_hash_digest from .contextmanagers import DraftChangeLogContext from .models import ( @@ -23,6 +24,7 @@ Draft, DraftChangeLog, DraftChangeLogRecord, + DraftDependency, DraftSideEffect, EntityList, EntityListRow, @@ -34,6 +36,7 @@ PublishableEntityVersionMixin, PublishLog, PublishLogRecord, + PublishSideEffect, ) from .models.publish_log import Published @@ -364,11 +367,13 @@ def publish_from_drafts( # If the drafts include any containers, we need to auto-publish their descendants: # TODO: this only handles one level deep and would need to be updated to support sections > subsections > units + # note to self; manytomany through table for + # dependencies = draft_qset.filter(dependency_set) + # Get the IDs of the ContainerVersion for any Containers whose drafts are slated to be published. - container_version_ids = ( - Container.objects.filter(publishable_entity__draft__in=draft_qset) - .values_list("publishable_entity__draft__version__containerversion__pk", flat=True) - ) + container_version_ids = draft_qset.filter(entity__container__isnull=False) \ + .values_list("version_id", flat=True) + if container_version_ids: # We are publishing at least one container. Check if it has any child components that aren't already slated # to be published. @@ -422,9 +427,21 @@ def publish_from_drafts( }, ) + # Calculate the side-effects... + _create_side_effects_for_publish_log(publish_log) + + # Calculate state updates... + _update_state_hash(publish_log) + return publish_log +def _update_state_hash(publish_log: PublishLog) -> None: + """ + Find all records where + """ + + def get_draft_version(publishable_entity_id: int, /) -> PublishableEntityVersion | None: """ Return current draft PublishableEntityVersion for this PublishableEntity. @@ -556,7 +573,7 @@ def set_draft_version( old_version_id=old_version_id, new_version_id=publishable_entity_version_pk, ) - _create_container_side_effects_for_draft_change(change) + _create_side_effects_for_draft_change(change) def _add_to_existing_draft_change_log( @@ -622,24 +639,107 @@ def _add_to_existing_draft_change_log( return change -def _create_container_side_effects_for_draft_change_log(change_log: DraftChangeLog): +def _create_side_effects_for_publish_log(publish_log: PublishLog): + """ + Iterate through PublishLog and add the appropriate PublishSideEffects. + + A container is considered to have been "published" if any of its descendents + were published. So publishing a Component implicitly publishes any Units it + was in, which publishes the Subsections those Units were in, etc. If the + Container is not already in the PublishLog (i.e. its own metadata did not + change), then we create a PublishLogRecord where the old_version is equal + to the new_version. We also create a PublishSideEffect for every child + change that affects a container, regardless of whether that container also + changed. + + This is a slightly simplified version of the work we have to do with Drafts, + because we don't have to worry about being called from a bulk context or + whether we're calculating for an entire log or just one log record. + + Note: I first tried to combine this logic with its draft-related counterpart + because of their obvious similaries, but I felt that the resulting code was + overly confusing, and that it was better to just repeat ourselves slightly, + rather than to try to parameterize things like the model classes, and have + ugly if-else blocks to work out the small differences. + """ + # processed_entity_ids 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 each time through the loop. + # It also guards against infinite parent-child relationship loops, though + # those aren't *supposed* to be allowed anyhow. + processed_entity_ids: set[int] = set() + for original_change in publish_log.records.all(): + changes_and_containers = [ + (original_change, container) + for container + in get_containers_with_entity( + original_change.entity_id, + ignore_pinned=True, + published=True, + ) + ] + while changes_and_containers: + change, container = changes_and_containers.pop() + + # If the container is not already in the PublishLog, we need to + # add it. Since it's being caused as a PublishSideEffect, we're + # going add it with the old_version == new_version convention, i.e. + # the *only* change is because of one its children. + container_published_version_pk = container.versioning.published.pk + container_change, _created = PublishLogRecord.objects.get_or_create( + publish_log=change.publish_log, + entity_id=container.pk, + defaults={ + 'old_version_id': container_published_version_pk, + 'new_version_id': container_published_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). + PublishSideEffect.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 Subsections + # 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 _create_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( + _create_side_effects_for_draft_change( change, processed_entity_ids=processed_entity_ids, ) -def _create_container_side_effects_for_draft_change( +def _create_side_effects_for_draft_change( original_change: DraftChangeLogRecord, processed_entity_ids: set | None = None ): """ - Given a draft change, add side effects for all affected containers. + Given a draft change, add side effects for all affected Drafts. This should only be run after the DraftChangeLogRecord has been otherwise fully written out. We want to avoid the scenario where we create a @@ -652,32 +752,34 @@ def _create_container_side_effects_for_draft_change( 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. """ if processed_entity_ids is None: # An optimization, but also a guard against infinite side-effect loops. processed_entity_ids = set() - changes_and_containers = [ - (original_change, container) - for container - in get_containers_with_entity(original_change.entity_id, ignore_pinned=True) + # This is basically original_change.entity.draft.causes_side_effects_for, + # but faster and safer (handles deleted drafts). + target_dependencies = ( + DraftDependency.objects + .filter(dependency_id=original_change.entity_id) + .select_related("draft") + ) + changes_and_side_effect_target_drafts = [ + (original_change, target_dep.target) for target_dep in target_dependencies ] - while changes_and_containers: - change, container = changes_and_containers.pop() + while changes_and_side_effect_target_drafts: + change, draft = changes_and_side_effect_target_drafts.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( + target_draft_version_pk = draft.version_id + target_change, _created = DraftChangeLogRecord.objects.get_or_create( draft_change_log=change.draft_change_log, - entity_id=container.pk, + entity_id=draft.pk, defaults={ - 'old_version_id': container_draft_version_pk, - 'new_version_id': container_draft_version_pk + 'old_version_id': target_draft_version_pk, + 'new_version_id': target_draft_version_pk } ) @@ -687,21 +789,134 @@ def _create_container_side_effects_for_draft_change( # 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) + DraftSideEffect.objects.get_or_create(cause=change, effect=target_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 + # is the Unit, and ``parents_of_container`` would be any Subsections # that contain the Unit. - parents_of_container = get_containers_with_entity(container.pk, ignore_pinned=True) + target_deps_of_draft = list(draft.causes_side_effects_for.all().select_related("draft")) + + changes_and_side_effect_target_drafts.extend( + (target_change, target_dep.draft) + for target_dep in target_deps_of_draft + if target_dep.pk not in processed_entity_ids + ) + + +def set_draft_dependencies(draft: Draft, dependency_drafts: QuerySet[Draft]) -> None: + """ + Declare the set of Drafts that this Draft depends on. + + This call *replaces* whatever the last set of dependencies for this Draft + was, so you should only call it once when the draft's dependencies change. - changes_and_containers.extend( - (container_change, container_parent) - for container_parent in parents_of_container - if container_parent.pk not in processed_entity_ids + Something is a dependency if a change in it is considered to be a change in + the ``draft``, even if ``draft`` itself does not change. For example, a + Unit does not get a new version when unpinned child Components are updated, + but we do consider a Unit to be changed when that happens. So the Drafts of + the child Components would be dependencies of the Unit. + + 1. Only declare one level of dependencies, e.g. immediate parent-child + relationships. The publishing app can calculate transitive dependencies + like "all descendants" based on this. + 2. Declare dependencies from the bottom-up. In other words, if you're + building an entire Subsection, set the Component dependencies for the + Units before you set the Unit dependencies for the Subsection. This code + will still work if you build from the top-down, but we'll end up doing + many redundant re-calculations, since every change to a lower layer will + cause recalculation to the higher levels that depend on it. + 3. Circular dependencies are not supported, and will raise a ``ValueError``. + """ + new_dependency_drafts = ( + dependency_drafts.exclude(pk=draft.pk) # draft can't depend on itself + .distinct() # duplicates not allowed + ) + + # We're going to delete any DraftDependency rows that currently exist for + # this draft but aren't represented in the new_dependency_drafts Draft qset. + old_dependencies: QuerySet[DraftDependency] = draft.dependencies.all() + dependencies_to_delete = old_dependencies.exclude( + dependency__in=new_dependency_drafts.values_list("pk", flat=True) + ) + dependency_drafts_to_add = new_dependency_drafts.exclude( + pk__in=old_dependencies.values_list("dependency_id", flat=True) + ) + with atomic(savepoint=False): + dependencies_to_delete.delete() + DraftDependency.objects.bulk_create( + [ + DraftDependency( + target=draft, + dependency_id=dependency_draft.pk, + version_at_creation_id=draft.version_id, + ) + for dependency_draft in dependency_drafts_to_add + ], + # If a conflict exists, then we want to ignore instead of update, + # because our value for "version_at_creation" will be incorrect. + ignore_conflicts=True, ) + draft.state_hash = _calcuate_state_hash(draft) + + # We've updated this Draft's state_hash based on its dependencies, but + # now we have to re-calculate the state_hash for things that are + # dependent on it. + updated_draft_pks = {draft.pk} + drafts_with_outdated_state = list(draft.causes_side_effects_for.all()) + while drafts_with_outdated_state: + draft_with_outdated_state = drafts_with_outdated_state.pop() + if draft_with_outdated_state.pk in updated_draft_pks: + raise ValueError( + f"Cannot set draft dependencies for {draft} to " + f"{list(dependency_drafts_to_add)} because it introduces a " + "circular dependency." + ) + + draft_with_outdated_state.state_hash = _calcuate_state_hash(draft_with_outdated_state) + drafts_with_outdated_state.extend( + dep.target for dep in draft_with_outdated_state.causes_side_effects_for.all().select_related("target") + ) + + +def _calcuate_state_hash(draft: Draft) -> str: + """ + Generate a new value that can be put in Draft.state_hash. + + We calculate the state hash based on the versions (UUIDs) and state_hash + values of all of its dependencies. There is no sense of ordering with these + dependencies. A Unit with Components [C1, C2] is the same to us as a Unit + with [C2, C1], and both will have the same state_hash. We rely on the fact + that containers store their ordering and that changes in that ordering mean + that the version of the container itself (e.g. the Unit) changes. + + Edge case: Deleted Drafts (i.e. draft.version == None) + + When this happens, the deleted Draft still counts as a dependency because + un-deleting it will cause a side-effect. But it has no version UUID to use + in state calculation, so we skip it entirely. We can safely skip this even + if there are transitive dependencies (e.g. a deleted Unit with undeleted + Components), because changes in those transitive Component dependencies + won't affect the top level Draft until the Unit is un-deleted, at which + point we'd have to recalcuate the state_hash anyway. + """ + dependencies: QuerySet[DraftDependency] = ( + draft.dependencies + .all() + .select_related("dependency__version") + # need to guarantee order for deterministic state calculation + .order_by("dependency__version__uuid") + ) + dependency_states = [ + f"{dd.dependency.version.uuid}:{dd.dependency.state_hash}" + for dd in dependencies + if dd.dependency.version # skip deleted drafts + ] + # Example line: [TODO: Fill this in] + dependency_summary_text = "\n".join(dependency_states) + return create_hash_digest(dependency_summary_text.encode()) def soft_delete_draft(publishable_entity_id: int, /, deleted_by: int | None = None) -> None: @@ -1045,6 +1260,12 @@ def create_container_version( created_by=created_by, container_version_cls=container_version_cls, ) + set_draft_dependencies( + Draft.objects.get(pk=container_id), + Draft.objects.filter( + pk__in=[entity_row.entity_pk for entity_row in entity_rows] + ) + ) return container_version @@ -1393,6 +1614,7 @@ def get_containers_with_entity( publishable_entity_pk: int, *, ignore_pinned=False, + published=False, ) -> QuerySet[Container]: """ [ 🛑 UNSTABLE ] @@ -1405,20 +1627,31 @@ def get_containers_with_entity( publishable_entity_pk: The ID of the PublishableEntity to search for. ignore_pinned: if true, ignore any pinned references to the entity. """ + relation_model = "published" if published else "draft" if ignore_pinned: - 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 - publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_version_id=None, # pylint: disable=line-too-long # noqa: E501 - ) + filter_dict = { + # Note: these two conditions must be in the same filter() call, + # or the query won't be correct. + ( + f"publishable_entity__{relation_model}__version__" + "containerversion__entity_list__entitylistrow__entity_id" + ): publishable_entity_pk, + ( + f"publishable_entity__{relation_model}__version__" + "containerversion__entity_list__entitylistrow__entity_version_id" + ): None, + } + qs = Container.objects.filter(**filter_dict) else: - qs = Container.objects.filter( - publishable_entity__draft__version__containerversion__entity_list__entitylistrow__entity_id=publishable_entity_pk, # pylint: disable=line-too-long # noqa: E501 - ) - return qs.select_related( - "publishable_entity__draft__version__containerversion", - "publishable_entity__published__version__containerversion", - ).order_by("pk").distinct() # Ordering is mostly for consistent test cases. + filter_dict = { + ( + f"publishable_entity__{relation_model}__version__" + "containerversion__entity_list__entitylistrow__entity_id" + ): publishable_entity_pk + } + qs = Container.objects.filter(**filter_dict) + + return qs.order_by("pk").distinct() # Ordering is mostly for consistent test cases. def get_container_children_count( @@ -1486,6 +1719,6 @@ def bulk_draft_changes_for( changed_at=changed_at, changed_by=changed_by, exit_callbacks=[ - _create_container_side_effects_for_draft_change_log, + _create_side_effects_for_draft_change_log, ] ) diff --git a/openedx_learning/apps/authoring/publishing/migrations/0009_create_publishsideeffect.py b/openedx_learning/apps/authoring/publishing/migrations/0009_create_publishsideeffect.py new file mode 100644 index 000000000..03b3208b3 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0009_create_publishsideeffect.py @@ -0,0 +1,86 @@ +# Generated by Django 4.2.16 on 2025-04-17 08:47 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("oel_publishing", "0008_alter_draftchangelogrecord_options_and_more"), + ] + + operations = [ + migrations.AlterModelOptions( + name="draftchangelogrecord", + options={ + "verbose_name": "Draft Change Log Record", + "verbose_name_plural": "Draft Change Log Records", + }, + ), + migrations.AlterModelOptions( + name="draftsideeffect", + options={ + "verbose_name": "Draft Side Effect", + "verbose_name_plural": "Draft Side Effects", + }, + ), + migrations.AlterField( + model_name="draftchangelogrecord", + name="draft_change_log", + field=models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="records", + to="oel_publishing.draftchangelog", + ), + ), + migrations.AlterField( + model_name="draftsideeffect", + name="effect", + field=models.ForeignKey( + on_delete=django.db.models.deletion.RESTRICT, + related_name="affected_by", + to="oel_publishing.draftchangelogrecord", + ), + ), + migrations.CreateModel( + name="PublishSideEffect", + 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.publishlogrecord", + ), + ), + ( + "effect", + models.ForeignKey( + on_delete=django.db.models.deletion.RESTRICT, + related_name="affected_by", + to="oel_publishing.publishlogrecord", + ), + ), + ], + options={ + "verbose_name": "Publish Side Effect", + "verbose_name_plural": "Publish Side Effects", + }, + ), + migrations.AddConstraint( + model_name="publishsideeffect", + constraint=models.UniqueConstraint( + fields=("cause", "effect"), name="oel_pub_pse_uniq_c_e" + ), + ), + ] diff --git a/openedx_learning/apps/authoring/publishing/migrations/0010_draft_dependency.py b/openedx_learning/apps/authoring/publishing/migrations/0010_draft_dependency.py new file mode 100644 index 000000000..63978a257 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0010_draft_dependency.py @@ -0,0 +1,32 @@ +# Generated by Django 4.2.21 on 2025-06-03 20:40 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0009_create_publishsideeffect'), + ] + + operations = [ + migrations.AddField( + model_name='draft', + name='state_hash', + field=models.CharField(blank=True, default='', editable=False, max_length=40), + ), + migrations.CreateModel( + name='DraftDependency', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('dependency', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='causes_side_effects_for', to='oel_publishing.draft')), + ('draft', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='dependencies', to='oel_publishing.draft')), + ('version_at_creation', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, related_name='draft_dependencies_created', to='oel_publishing.publishableentityversion')), + ], + ), + migrations.AddConstraint( + model_name='draftdependency', + constraint=models.UniqueConstraint(fields=('draft', 'dependency'), name='oel_pub_dd_uniq_draft_dep'), + ), + ] diff --git a/openedx_learning/apps/authoring/publishing/migrations/0011_remove_draftdependency_oel_pub_dd_uniq_draft_dep_and_more.py b/openedx_learning/apps/authoring/publishing/migrations/0011_remove_draftdependency_oel_pub_dd_uniq_draft_dep_and_more.py new file mode 100644 index 000000000..a2458b324 --- /dev/null +++ b/openedx_learning/apps/authoring/publishing/migrations/0011_remove_draftdependency_oel_pub_dd_uniq_draft_dep_and_more.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.21 on 2025-06-07 00:56 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0010_draft_dependency'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='draftdependency', + name='oel_pub_dd_uniq_draft_dep', + ), + migrations.RenameField( + model_name='draftdependency', + old_name='draft', + new_name='target', + ), + migrations.AddConstraint( + model_name='draftdependency', + constraint=models.UniqueConstraint(fields=('target', 'dependency'), name='oel_pub_dd_uniq_target_dep'), + ), + ] diff --git a/openedx_learning/apps/authoring/publishing/models/__init__.py b/openedx_learning/apps/authoring/publishing/models/__init__.py index 7e13c426a..380874904 100644 --- a/openedx_learning/apps/authoring/publishing/models/__init__.py +++ b/openedx_learning/apps/authoring/publishing/models/__init__.py @@ -14,10 +14,16 @@ """ from .container import Container, ContainerVersion -from .draft_log import Draft, DraftChangeLog, DraftChangeLogRecord, DraftSideEffect +from .draft_log import ( + Draft, + DraftChangeLog, + DraftChangeLogRecord, + DraftDependency, + DraftSideEffect, +) from .entity_list import EntityList, EntityListRow from .learning_package import LearningPackage -from .publish_log import Published, PublishLog, PublishLogRecord +from .publish_log import Published, PublishLog, PublishLogRecord, PublishSideEffect 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 index 6e85a6ec8..7b88ec5aa 100644 --- a/openedx_learning/apps/authoring/publishing/models/draft_log.py +++ b/openedx_learning/apps/authoring/publishing/models/draft_log.py @@ -5,7 +5,11 @@ 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 openedx_learning.lib.fields import ( + hash_field, + immutable_uuid_field, + manual_date_time_field, +) from .learning_package import LearningPackage from .publishable_entity import PublishableEntity, PublishableEntityVersion @@ -56,6 +60,19 @@ class Draft(models.Model): blank=True, ) + # The state_hash is used when the version alone isn't enough to let us know + # the full draft state of an entity. This happens any time a Draft has + # dependencies (see the DraftDependency model), because changes in those + # dependencies will cause changes to the state of the Draft. An example of + # this is contianers, where changing an unpinned child affects the state of + # the parent container, even if that container's definition (and thus + # version) does not change. + # + # If a Draft has no dependencies, then its entire state is captured by its + # version, and the state_hash is blank. (Blank is slightly more convenient + # for database comparisons than NULL.) + state_hash = hash_field(blank=True, default='') + class DraftChangeLog(models.Model): """ @@ -313,3 +330,46 @@ class Meta: ] verbose_name = _("Draft Side Effect") verbose_name_plural = _("Draft Side Effects") + + +# This needs to go in the Draft model? +# The new_state_hash is used when the version alone isn't enough to let us +# know the full published state of an entity. This happens with Containers, +# where the published state of the container can change because its children +# are published. +# state_hash = hash_field(blank=True, default='') +# +# It needs to exist in the Published model too, but we *can't assume it'll copy* +# because different subsets of children could be published, so we need to be +# able to recalculate based on the published state of the dependencies. + + +class DraftDependency(models.Model): + """ + + """ + target = models.ForeignKey( + Draft, + on_delete=models.CASCADE, + related_name="dependencies", + ) + dependency = models.ForeignKey( + Draft, + on_delete=models.CASCADE, + related_name="causes_side_effects_for", + ) + version_at_creation = models.ForeignKey( + PublishableEntityVersion, + related_name="draft_dependencies_created", + on_delete=models.RESTRICT, + ) + + class Meta: + constraints = [ + # Duplicate entries for a dependency are just redundant. This is + # here to guard against weird bugs that might introduce this state. + models.UniqueConstraint( + fields=["target", "dependency"], + name="oel_pub_dd_uniq_target_dep", + ) + ] \ No newline at end of file diff --git a/openedx_learning/apps/authoring/publishing/models/publish_log.py b/openedx_learning/apps/authoring/publishing/models/publish_log.py index 037f1b8b1..dadfb8535 100644 --- a/openedx_learning/apps/authoring/publishing/models/publish_log.py +++ b/openedx_learning/apps/authoring/publishing/models/publish_log.py @@ -3,8 +3,13 @@ """ from django.conf import settings from django.db import models +from django.utils.translation import gettext_lazy as _ -from openedx_learning.lib.fields import case_insensitive_char_field, immutable_uuid_field, manual_date_time_field +from openedx_learning.lib.fields import ( + case_insensitive_char_field, + immutable_uuid_field, + manual_date_time_field, +) from .learning_package import LearningPackage from .publishable_entity import PublishableEntity, PublishableEntityVersion @@ -61,6 +66,15 @@ class PublishLogRecord(models.Model): To revert a publish, we would make a new publish that swaps ``old_version`` and ``new_version`` field values. + + If the old_version and new_version of a PublishLogRecord match, it means + that the definition of the entity itself did not change (i.e. no new + PublishableEntityVersion was created), but something else was published that + had the side-effect of changing the published state of this entity. For + instance, if a Unit has unpinned references to its child Components (which + it almost always will), then publishing one of those Components will alter + the published state of the Unit, even if the UnitVersion does not change. In + that case, we still consider the Unit to have been "published". """ publish_log = models.ForeignKey( @@ -148,3 +162,51 @@ class Published(models.Model): class Meta: verbose_name = "Published Entity" verbose_name_plural = "Published Entities" + + +class PublishSideEffect(models.Model): + """ + Model to track when a change in one Published entity affects others. + + 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 published and they are part of Unit U1, + which is in turn a part of Subsection SS1, then the PublishSideEffect + 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. + .. no_pii: + """ + cause = models.ForeignKey( + PublishLogRecord, + on_delete=models.RESTRICT, + related_name='causes', + ) + effect = models.ForeignKey( + PublishLogRecord, + on_delete=models.RESTRICT, + related_name='affected_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_pse_uniq_c_e", + ) + ] + verbose_name = _("Publish Side Effect") + verbose_name_plural = _("Publish Side Effects") + + + diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 7256c7060..4315d6789 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -177,22 +177,21 @@ def create_unit_and_version( can_stand_alone: Set to False when created as part of containers """ entity_rows = _pub_entities_for_components(components) - with atomic(): - unit = create_unit( - learning_package_id, - key, - created, - created_by, - can_stand_alone=can_stand_alone, - ) - unit_version = create_unit_version( - unit, - 1, - title=title, - entity_rows=entity_rows or [], - created=created, - created_by=created_by, - ) + unit = create_unit( + learning_package_id, + key, + created, + created_by, + can_stand_alone=can_stand_alone, + ) + unit_version = create_unit_version( + unit, + 1, + title=title, + entity_rows=entity_rows or [], + created=created, + created_by=created_by, + ) return unit, unit_version diff --git a/openedx_learning/lib/fields.py b/openedx_learning/lib/fields.py index 99b7d5251..56bd3d634 100644 --- a/openedx_learning/lib/fields.py +++ b/openedx_learning/lib/fields.py @@ -123,7 +123,7 @@ def key_field(**kwargs) -> MultiCollationCharField: return case_sensitive_char_field(max_length=500, blank=False, **kwargs) -def hash_field() -> models.CharField: +def hash_field(**kwargs) -> models.CharField: """ Holds a hash digest meant to identify a piece of content. @@ -144,13 +144,13 @@ def hash_field() -> models.CharField: didn't seem worthwhile, particularly the possibility of case-sensitivity related bugs. """ - return models.CharField( - max_length=40, - blank=False, - null=False, - editable=False, - ) - + default_kwargs = { + "max_length": 40, + "blank": False, + "null": False, + "editable": False, + } + return models.CharField(**(default_kwargs | kwargs)) def manual_date_time_field() -> models.DateTimeField: """ diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 94c328f22..7f981a067 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -19,6 +19,7 @@ DraftSideEffect, LearningPackage, PublishableEntity, + PublishLog, ) from openedx_learning.lib.test_utils import TestCase @@ -832,6 +833,108 @@ def test_multiple_draft_changes_all_cancel_out(self) -> None: assert DraftChangeLog.objects.all().count() == 1 +class PublishLogTestCase(TestCase): + """ + Test basic operations with PublishLogs and PublishSideEffects. + """ + 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", + "PublishLog Testing LearningPackage 🔥 1", + created=cls.now, + ) + cls.learning_package_2 = publishing_api.create_learning_package( + "my_package_key_2", + "PublishLog Testing LearningPackage 🔥 2", + created=cls.now, + ) + + def test_simple_publish_log(self) -> None: + """ + Simplest test that multiple writes make it into one PublishLog. + """ + with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): + entity1 = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity", + created=self.now, + created_by=None, + ) + entity1_v1 = publishing_api.create_publishable_entity_version( + entity1.id, + version_num=1, + title="An Entity 🌴", + created=self.now, + created_by=None, + ) + entity2 = publishing_api.create_publishable_entity( + self.learning_package_1.id, + "my_entity2", + created=self.now, + created_by=None, + ) + entity2_v1 = publishing_api.create_publishable_entity_version( + entity2.id, + version_num=1, + title="An Entity 🌴 2", + created=self.now, + created_by=None, + ) + + # Check simplest publish of two things... + publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id) + assert PublishLog.objects.all().count() == 1 + assert publish_log.records.all().count() == 2 + + record_1 = publish_log.records.get(entity=entity1) + assert record_1 is not None + assert record_1.old_version is None + assert record_1.new_version == entity1_v1 + + record_2 = publish_log.records.get(entity=entity2) + assert record_2 is not None + assert record_2.old_version is None + assert record_2.new_version == entity2_v1 + + # Check that an empty publish still creates a PublishLog, just one with + # no records. This may be useful for triggering tasks that trigger off + # of publishing events, though I'm not confident about that at the + # moment. + publish_log = publishing_api.publish_all_drafts(self.learning_package_1.id) + assert publish_log.records.count() == 0 + + # Check that we can publish a subset... + entity1_v2 = publishing_api.create_publishable_entity_version( + entity1.id, + version_num=2, + title="An Entity 🌴", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity2.id, + version_num=2, + title="An Entity 🌴 2", + created=self.now, + created_by=None, + ) + publish_e1_log = publishing_api.publish_from_drafts( + self.learning_package_1.id, + Draft.objects.filter(pk=entity1.pk), + ) + assert publish_e1_log.records.count() == 1 + e1_pub_record = publish_e1_log.records.get(entity=entity1) + assert e1_pub_record is not None + assert e1_pub_record.old_version == entity1_v1 + assert e1_pub_record.new_version == entity1_v2 + + class ContainerTestCase(TestCase): """ Test basic operations with Drafts. @@ -1076,6 +1179,72 @@ def test_multiple_layers_of_containers(self): assert subsection_change.affected_by.count() == 1 assert subsection_change.affected_by.first().cause == unit_change + publish_log = publishing_api.publish_all_drafts(self.learning_package.id) + assert publish_log.records.count() == 3 + + publishing_api.create_publishable_entity_version( + component.pk, version_num=3, title="Component v2", created=self.now, created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package.id, + Draft.objects.filter(entity_id=component.pk), + ) + assert publish_log.records.count() == 3 + component_publish = publish_log.records.get(entity=component) + unit_publish = publish_log.records.get(entity=unit.publishable_entity) + subsection_publish = publish_log.records.get(entity=subsection.publishable_entity) + + assert not component_publish.affected_by.exists() + assert unit_publish.affected_by.count() == 1 + assert unit_publish.affected_by.first().cause == component_publish + assert subsection_publish.affected_by.count() == 1 + assert subsection_publish.affected_by.first().cause == unit_publish + + def test_publish_all_layers(self): + """Test that we can publish multiple layers from one root.""" + # 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, + ) + 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, + ) + publishing_api.create_container_version( + unit.pk, + 1, + title="My Unit", + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=component.pk), + ], + created=self.now, + created_by=None, + ) + subsection = publishing_api.create_container( + self.learning_package.id, "subsection_1", created=self.now, created_by=None, + ) + publishing_api.create_container_version( + subsection.pk, + 1, + title="My Subsection", + entity_rows=[ + publishing_api.ContainerEntityRow(entity_pk=unit.pk), + ], + created=self.now, + created_by=None, + ) + publish_log = publishing_api.publish_from_drafts( + self.learning_package.id, + Draft.objects.filter(pk=subsection.pk), + ) + assert publish_log.records.count() == 3 + + class EntitiesQueryTestCase(TestCase): """ diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index 4b269d3ef..1f97c9745 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -214,9 +214,9 @@ def test_create_section_queries(self): Test how many database queries are required to create a section """ # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with self.assertNumQueries(25): + with self.assertNumQueries(29): _empty_section = self.create_section_with_subsections([]) - with self.assertNumQueries(30): + with self.assertNumQueries(36): # And try with a non-empty section: self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1], key="u2") diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py index 847336611..fe850d430 100644 --- a/tests/openedx_learning/apps/authoring/subsections/test_api.py +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -197,9 +197,9 @@ def test_create_subsection_queries(self): Test how many database queries are required to create a subsection """ # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with self.assertNumQueries(25): + with self.assertNumQueries(29): _empty_subsection = self.create_subsection_with_units([]) - with self.assertNumQueries(30): + with self.assertNumQueries(36): # And try with a non-empty subsection: self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2") diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index 3c3caa7d9..d3cf10f6f 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -185,9 +185,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(25): + with self.assertNumQueries(27): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(29): + with self.assertNumQueries(33): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2")