From 9cf955f8cc4d451a86cb3f814cd690ec1206a34a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 10 Mar 2025 11:09:24 -0700 Subject: [PATCH 1/7] refactor: Units are Containers --- .../apps/authoring/publishing/api.py | 49 +++------ .../publishing/migrations/0003_containers.py | 5 +- .../publishing/model_mixins/__init__.py | 1 - .../publishing/model_mixins/container.py | 98 ----------------- .../authoring/publishing/models/container.py | 91 +++++++++++++++ openedx_learning/apps/authoring/units/api.py | 74 ++++++------- .../units/migrations/0001_initial.py | 20 ++-- .../apps/authoring/units/models.py | 24 ++-- .../apps/authoring/units/test_api.py | 104 ++++++++++-------- 9 files changed, 228 insertions(+), 238 deletions(-) delete mode 100644 openedx_learning/apps/authoring/publishing/model_mixins/container.py diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 895f091f..5fece0ac 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -13,12 +13,7 @@ from django.db.models import F, Q, QuerySet from django.db.transaction import atomic -from .model_mixins import ( - ContainerMixin, - PublishableContentModelRegistry, - PublishableEntityMixin, - PublishableEntityVersionMixin, -) +from .model_mixins import PublishableContentModelRegistry, PublishableEntityMixin, PublishableEntityVersionMixin from .models import ( Container, ContainerVersion, @@ -579,6 +574,7 @@ def get_published_version_as_of(entity_id: int, publish_log_id: int) -> Publisha def create_container( learning_package_id: int, key: str, + container_type: str, created: datetime, created_by: int | None, ) -> Container: @@ -595,12 +591,14 @@ def create_container( Returns: The newly created container. """ + assert container_type # Shouldn't be empty/none with atomic(): publishable_entity = create_publishable_entity( learning_package_id, key, created, created_by ) container = Container.objects.create( publishable_entity=publishable_entity, + container_type=container_type, ) return container @@ -635,7 +633,7 @@ def create_entity_list_with_rows( The newly created entity list. """ order_nums = range(len(entity_pks)) - with atomic(): + with atomic(savepoint=False): entity_list = create_entity_list() EntityListRow.objects.bulk_create( [ @@ -679,7 +677,7 @@ def create_container_version( Returns: The newly created container version. """ - with atomic(): + with atomic(savepoint=False): container = Container.objects.select_related("publishable_entity").get(pk=container_pk) entity = container.publishable_entity @@ -789,6 +787,7 @@ def create_container_and_version( learning_package_id: int, key: str, *, + container_type: str, created: datetime, created_by: int | None, title: str, @@ -812,7 +811,7 @@ def create_container_and_version( The newly created container version. """ with atomic(): - container = create_container(learning_package_id, key, created, created_by) + container = create_container(learning_package_id, key, container_type, created, created_by) container_version = create_container_version( container.publishable_entity.pk, 1, @@ -854,15 +853,13 @@ def entity(self): def get_entities_in_draft_container( - container: Container | ContainerMixin, + container: Container, ) -> list[ContainerEntityListEntry]: """ [ 🛑 UNSTABLE ] Get the list of entities and their versions in the draft version of the given container. """ - if isinstance(container, ContainerMixin): - container = container.container assert isinstance(container, Container) entity_list = [] for row in container.versioning.draft.entity_list.entitylistrow_set.order_by("order_num"): @@ -877,19 +874,15 @@ def get_entities_in_draft_container( def get_entities_in_published_container( - container: Container | ContainerMixin, + container: Container, ) -> list[ContainerEntityListEntry] | None: """ [ 🛑 UNSTABLE ] Get the list of entities and their versions in the published version of the given container. """ - if isinstance(container, ContainerMixin): - cv = container.container.versioning.published - elif isinstance(container, Container): - cv = container.versioning.published - else: - raise TypeError(f"Expected Container or ContainerMixin; got {type(container)}") + assert isinstance(container, Container) + cv = container.versioning.published if cv is None: return None # There is no published version of this container. Should this be an exception? assert isinstance(cv, ContainerVersion) @@ -905,9 +898,7 @@ def get_entities_in_published_container( return entity_list -def contains_unpublished_changes( - container: Container | ContainerMixin, -) -> bool: +def contains_unpublished_changes(container_id: int) -> bool: """ [ 🛑 UNSTABLE ] Check recursively if a container has any unpublished changes. @@ -920,14 +911,10 @@ def contains_unpublished_changes( that's in the container, it will be `False`. This method will return `True` in either case. """ - if isinstance(container, ContainerMixin): - # This is similar to 'get_container(container.container_id)' but pre-loads more data. - container = Container.objects.select_related( - "publishable_entity__draft__version__containerversion__entity_list", - ).get(pk=container.container_id) - else: - pass # TODO: select_related if we're given a raw Container rather than a ContainerMixin like Unit? - assert isinstance(container, Container) + # This is similar to 'get_container(container.container_id)' but pre-loads more data. + container = Container.objects.select_related( + "publishable_entity__draft__version__containerversion__entity_list", + ).get(pk=container_id) if container.versioning.has_unpublished_changes: return True @@ -949,7 +936,7 @@ def contains_unpublished_changes( child_container = None if child_container: # This is itself a container - check recursively: - if contains_unpublished_changes(child_container): + if contains_unpublished_changes(child_container.pk): return True else: # This is not a container: diff --git a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py index 7260e2a0..8048017e 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py @@ -1,8 +1,10 @@ -# Generated by Django 4.2.19 on 2025-03-07 23:09 +# Generated by Django 4.2.19 on 2025-03-10 18:03 import django.db.models.deletion from django.db import migrations, models +import openedx_learning.lib.fields + class Migration(migrations.Migration): @@ -15,6 +17,7 @@ class Migration(migrations.Migration): name='Container', fields=[ ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), + ('container_type', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/publishing/model_mixins/__init__.py b/openedx_learning/apps/authoring/publishing/model_mixins/__init__.py index 47f90297..14ea9315 100644 --- a/openedx_learning/apps/authoring/publishing/model_mixins/__init__.py +++ b/openedx_learning/apps/authoring/publishing/model_mixins/__init__.py @@ -1,5 +1,4 @@ """ Mixins provided by the publishing app """ -from .container import * from .publishable_entity import * diff --git a/openedx_learning/apps/authoring/publishing/model_mixins/container.py b/openedx_learning/apps/authoring/publishing/model_mixins/container.py deleted file mode 100644 index 944d49ac..00000000 --- a/openedx_learning/apps/authoring/publishing/model_mixins/container.py +++ /dev/null @@ -1,98 +0,0 @@ -""" -ContainerMixin and ContainerVersionMixin -""" -from __future__ import annotations - -from datetime import datetime -from typing import TYPE_CHECKING, ClassVar, Self - -from django.db import models - -from openedx_learning.lib.managers import WithRelationsManager - -from .publishable_entity import PublishableEntityMixin, PublishableEntityVersionMixin - -if TYPE_CHECKING: - from ..models.container import Container, ContainerVersion -else: - # To avoid circular imports, we need to reference these models using strings only - Container = "oel_publishing.Container" - ContainerVersion = "oel_publishing.ContainerVersion" - -__all__ = [ - "ContainerMixin", - "ContainerVersionMixin", -] - - -class ContainerMixin(PublishableEntityMixin): - """ - Convenience mixin to link your models against Container. - - Please see docstring for Container for more details. - - If you use this class, you *MUST* also use ContainerVersionMixin - """ - - # select these related entities by default for all queries - objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] - "container", - "publishable_entity", - "publishable_entity__published", - "publishable_entity__draft", - ) - - container = models.OneToOneField( - Container, - on_delete=models.CASCADE, - ) - - @property - def uuid(self) -> str: - return self.container.uuid - - @property - def created(self) -> datetime: - return self.container.created - - class Meta: - abstract = True - - -class ContainerVersionMixin(PublishableEntityVersionMixin): - """ - Convenience mixin to link your models against ContainerVersion. - - Please see docstring for ContainerVersion for more details. - - If you use this class, you *MUST* also use ContainerMixin - """ - - # select these related entities by default for all queries - objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] - "container_version", - ) - - container_version = models.OneToOneField( - ContainerVersion, - on_delete=models.CASCADE, - ) - - @property - def uuid(self) -> str: - return self.container_version.uuid - - @property - def title(self) -> str: - return self.container_version.title - - @property - def created(self) -> datetime: - return self.container_version.created - - @property - def version_num(self): - return self.container_version.version_num - - class Meta: - abstract = True diff --git a/openedx_learning/apps/authoring/publishing/models/container.py b/openedx_learning/apps/authoring/publishing/models/container.py index 4a0df08a..0e4d9937 100644 --- a/openedx_learning/apps/authoring/publishing/models/container.py +++ b/openedx_learning/apps/authoring/publishing/models/container.py @@ -1,11 +1,54 @@ """ Container and ContainerVersion models """ +from typing import ClassVar, Self, TypeVar + +from django.core.exceptions import ValidationError from django.db import models +from openedx_learning.lib.fields import case_sensitive_char_field +from openedx_learning.lib.managers import WithRelationsManager + from ..model_mixins.publishable_entity import PublishableEntityMixin, PublishableEntityVersionMixin from .entity_list import EntityList +M = TypeVar('M', bound="Container") + + +class ContainerManager(WithRelationsManager[M]): + """ + A custom manager used for Container and its subclasses. + """ + def __init__(self): + """ + Initialize the manager for Container / a Container subclass + """ + super().__init__( + # Select these related entities by default: + "publishable_entity", + "publishable_entity__published", + "publishable_entity__draft", + ) + + def get_queryset(self) -> models.QuerySet: + """ + Apply filter() and select_related() to all querysets. + """ + qs = super().get_queryset() + if self.model.CONTAINER_TYPE: + qs = qs.filter(container_type=self.model.CONTAINER_TYPE) + return qs + + def create(self, **kwargs) -> M: + """ + Apply the values from our filter when creating new instances. + """ + if self.model.CONTAINER_TYPE: + # Don't allow creating via a subclass, like Unit.objects.create(). + # Instead use create_container() which calls Container.objects.create(..., container_type=...) + raise ValidationError("Container instances should only be created via APIs like create_container()") + return super().create(**kwargs) + class Container(PublishableEntityMixin): """ @@ -22,6 +65,40 @@ class Container(PublishableEntityMixin): PublishLog and Containers that were affected in a publish because their child elements were published. """ + # Subclasses (django proxy classes) should override this + CONTAINER_TYPE = "" + + objects: ClassVar[ContainerManager[Self]] = ContainerManager() # type: ignore[assignment] + + container_type = case_sensitive_char_field(max_length=500) + + def save(self, *args, **kwargs): + if not self.container_type: + raise ValidationError("Container instances should only be created via APIs like create_container()") + return super().save(*args, **kwargs) + + def clean(self): + """ + Validate this container subclass + """ + if self.container_type and self.CONTAINER_TYPE: + if self.container_type != self.CONTAINER_TYPE: + raise ValidationError("container type field mismatch with model.") + super().clean() + + @classmethod + def cast_from(cls, instance: "Container") -> Self: + """ + Create a new copy of a Container object, with a different subclass + """ + assert instance.container_type == cls.CONTAINER_TYPE + new_instance = cls( + pk=instance.pk, + container_type=instance.container_type, + ) + # Copy Django's internal cache of related objects + new_instance._state.fields_cache.update(instance._state.fields_cache) # pylint: disable=protected-access + return new_instance class ContainerVersion(PublishableEntityVersionMixin): @@ -58,3 +135,17 @@ class ContainerVersion(PublishableEntityVersionMixin): null=False, related_name="container_versions", ) + + @classmethod + def cast_from(cls, instance: "ContainerVersion") -> Self: + """ + Create a new copy of a Container object, with a different subclass + """ + new_instance = cls( + pk=instance.pk, + container_id=instance.container_id, + entity_list_id=instance.entity_list_id, + ) + # Copy Django's internal cache of related objects + new_instance._state.fields_cache.update(instance._state.fields_cache) # pylint: disable=protected-access + return new_instance diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index e18e6b50..57700ff2 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -41,15 +41,16 @@ def create_unit( created: The creation date. created_by: The user who created the unit. """ - with atomic(): - container = publishing_api.create_container( - learning_package_id, key, created, created_by - ) - unit = Unit.objects.create( - container=container, - publishable_entity=container.publishable_entity, - ) - return unit + container = publishing_api.create_container( + learning_package_id, + key, + Unit.CONTAINER_TYPE, + created, + created_by, + ) + return Unit.cast_from(container) + # Or, more robust but using additional queries: + # return Unit.objects.select_related(None).get(pk=container.pk) def create_unit_version( @@ -78,22 +79,17 @@ def create_unit_version( created: The creation date. created_by: The user who created the unit. """ - with atomic(): - container_version = publishing_api.create_container_version( - unit.container.pk, - version_num, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - unit_version = UnitVersion.objects.create( - unit=unit, - container_version=container_version, - publishable_entity_version=container_version.publishable_entity_version, - ) - return unit_version + container_version = publishing_api.create_container_version( + unit.pk, + version_num, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + ) + # return UnitVersion.objects.get(pk=container_version.pk) + return UnitVersion.cast_from(container_version) def _pub_entities_for_components( @@ -144,21 +140,15 @@ def create_next_unit_version( created_by: The user who created the unit. """ publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components) - with atomic(): - container_version = publishing_api.create_next_container_version( - unit.container.pk, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - unit_version = UnitVersion.objects.create( - unit=unit, - container_version=container_version, - publishable_entity_version=container_version.publishable_entity_version, - ) - return unit_version + container_version = publishing_api.create_next_container_version( + unit.pk, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + ) + return UnitVersion.cast_from(container_version) def create_unit_and_version( @@ -301,10 +291,10 @@ def get_components_in_published_unit_as_of( unit_pub_entity_version = publishing_api.get_published_version_as_of(unit.publishable_entity_id, publish_log_id) if unit_pub_entity_version is None: return None # This unit was not published as of the given PublishLog ID. - unit_version = unit_pub_entity_version.unitversion # type: ignore[attr-defined] + container_version = unit_pub_entity_version.containerversion entity_list = [] - rows = unit_version.container_version.entity_list.entitylistrow_set.order_by("order_num") + rows = container_version.entity_list.entitylistrow_set.order_by("order_num") for row in rows: if row.entity_version is not None: component_version = row.entity_version.componentversion diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py index 3bfc8a40..f3c8a477 100644 --- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py @@ -1,7 +1,6 @@ -# Generated by Django 4.2.19 on 2025-03-07 23:10 +# Generated by Django 4.2.19 on 2025-03-10 18:18 -import django.db.models.deletion -from django.db import migrations, models +from django.db import migrations class Migration(migrations.Migration): @@ -16,22 +15,23 @@ class Migration(migrations.Migration): migrations.CreateModel( name='Unit', fields=[ - ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.container')), ], options={ - 'abstract': False, + 'proxy': True, + 'indexes': [], + 'constraints': [], }, + bases=('oel_publishing.container',), ), migrations.CreateModel( name='UnitVersion', fields=[ - ('publishable_entity_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentityversion')), - ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.containerversion')), - ('unit', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_units.unit')), ], options={ - 'abstract': False, + 'proxy': True, + 'indexes': [], + 'constraints': [], }, + bases=('oel_publishing.containerversion',), ), ] diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 3a12e035..60ff982c 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -1,9 +1,8 @@ """ Models that implement units """ -from django.db import models -from ..publishing.model_mixins import ContainerMixin, ContainerVersionMixin +from ..publishing.models import Container, ContainerVersion __all__ = [ "Unit", @@ -11,21 +10,24 @@ ] -class Unit(ContainerMixin): +class Unit(Container): """ A Unit is Container, which is a PublishableEntity. """ + CONTAINER_TYPE = "unit" + class Meta: + proxy = True -class UnitVersion(ContainerVersionMixin): + +class UnitVersion(ContainerVersion): """ A UnitVersion is a ContainerVersion, which is a PublishableEntityVersion. """ - # Not sure what other metadata goes here, but we want to try to separate things - # like scheduling information and such into different models. - unit = models.ForeignKey( - Unit, - on_delete=models.CASCADE, - related_name="versions", - ) + @property + def unit(self): + return Unit.objects.get(pk=self.container_id) + + class Meta: + proxy = True diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index c063876d..0a3c7ce7 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -83,22 +83,41 @@ def test_get_unit(self): Test get_unit() """ unit = self.create_unit_with_components([self.component_1, self.component_2]) - result = authoring_api.get_unit(unit.pk) + with self.assertNumQueries(1): + result = authoring_api.get_unit(unit.pk) assert result == unit # Versioning data should be pre-loaded via select_related() with self.assertNumQueries(0): assert result.versioning.has_unpublished_changes - # TODO: (maybe) This currently has extra queries and is not preloaded even though it's the same: - # with self.assertNumQueries(0): - # assert result.container.versioning.has_unpublished_changes + + def test_get_unit_non_unit(self): + """ + Test that get_unit() cannot retrieve other container types + """ + not_unit, _version = authoring_api.create_container_and_version( + self.learning_package.id, + key="foobar", + container_type="NOT a unit", # <-- the important part + created=self.now, + created_by=None, + title="Testing", + publishable_entities_pks=[], + entity_version_pks=[] + ) + # This generic method will work: + authoring_api.get_container(not_unit.pk) + # But the unit method will not: + with self.assertRaises(authoring_models.Unit.DoesNotExist): + authoring_api.get_unit(not_unit.pk) def test_get_container(self): """ Test get_container() """ unit = self.create_unit_with_components([self.component_1, self.component_2]) - result = authoring_api.get_container(unit.container_id) - assert result == unit.container + with self.assertNumQueries(1): + result = authoring_api.get_container(unit.pk) + assert result == unit # Versioning data should be pre-loaded via select_related() with self.assertNumQueries(0): assert result.versioning.has_unpublished_changes @@ -108,9 +127,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(28): + with self.assertNumQueries(18): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(31): + with self.assertNumQueries(21): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2") @@ -127,6 +146,7 @@ def test_create_unit_with_invalid_children(self): created=self.now, created_by=None, ) + assert unit.versioning.draft == unit_version unit2, _u2v1 = authoring_api.create_unit_and_version( learning_package_id=self.learning_package.id, key="unit:key2", @@ -144,6 +164,8 @@ def test_create_unit_with_invalid_children(self): created_by=None, ) # Check that a new version was not created: + unit.refresh_from_db() + assert authoring_api.get_container(unit.pk).versioning.draft == unit_version assert unit.versioning.draft == unit_version def test_adding_external_components(self): @@ -272,7 +294,7 @@ def test_auto_publish_children(self): # Also create another component that's not in the unit at all: other_component, _oc_v1 = self.create_component(title="A draft component not in the unit", key="component:3") - assert authoring_api.contains_unpublished_changes(unit) + assert authoring_api.contains_unpublished_changes(unit.pk) assert self.component_1.versioning.published is None assert self.component_2.versioning.published is None @@ -286,7 +308,7 @@ def test_auto_publish_children(self): self.component_1.refresh_from_db() assert unit.versioning.has_unpublished_changes is False # Shallow check assert self.component_1.versioning.has_unpublished_changes is False - assert authoring_api.contains_unpublished_changes(unit) is False # Deep check + assert authoring_api.contains_unpublished_changes(unit.pk) is False # Deep check assert self.component_1.versioning.published == self.component_1_v1 # v1 is now the published version. # But our other component that's outside the unit is not affected: @@ -331,7 +353,7 @@ def test_add_component_after_publish(self): authoring_api.publish_all_drafts(self.learning_package.id) unit.refresh_from_db() # Reloading the unit is necessary assert unit.versioning.has_unpublished_changes is False # Shallow check for just the unit itself, not children - assert authoring_api.contains_unpublished_changes(unit) is False # Deeper check + assert authoring_api.contains_unpublished_changes(unit.pk) is False # Deeper check # Add a published component (unpinned): assert self.component_1.versioning.has_unpublished_changes is False @@ -345,7 +367,7 @@ def test_add_component_after_publish(self): # Now the unit should have unpublished changes: unit.refresh_from_db() # Reloading the unit is necessary assert unit.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the unit - assert authoring_api.contains_unpublished_changes(unit) # Deeper check + assert authoring_api.contains_unpublished_changes(unit.pk) # Deeper check assert unit.versioning.draft == unit_version_v2 assert unit.versioning.published == unit_version @@ -366,7 +388,7 @@ def test_modify_unpinned_component_after_publish(self): unit.refresh_from_db() # Reloading the unit is necessary if we accessed 'versioning' before publish self.component_1.refresh_from_db() assert unit.versioning.has_unpublished_changes is False # Shallow check - assert authoring_api.contains_unpublished_changes(unit) is False # Deeper check + assert authoring_api.contains_unpublished_changes(unit.pk) is False # Deeper check assert self.component_1.versioning.has_unpublished_changes is False # Now modify the component by changing its title (it remains a draft): @@ -376,7 +398,7 @@ def test_modify_unpinned_component_after_publish(self): unit.refresh_from_db() # Reloading the unit is necessary, or 'unit.versioning' will be outdated self.component_1.refresh_from_db() assert unit.versioning.has_unpublished_changes is False # Shallow check should be false - unit is unchanged - assert authoring_api.contains_unpublished_changes(unit) # But unit DOES contain changes + assert authoring_api.contains_unpublished_changes(unit.pk) # But unit DOES contain changes assert self.component_1.versioning.has_unpublished_changes # Since the component changes haven't been published, they should only appear in the draft unit @@ -395,7 +417,7 @@ def test_modify_unpinned_component_after_publish(self): assert authoring_api.get_components_in_published_unit(unit) == [ Entry(component_1_v2), # new version ] - assert authoring_api.contains_unpublished_changes(unit) is False # No longer contains unpublished changes + assert authoring_api.contains_unpublished_changes(unit.pk) is False # No longer contains unpublished changes def test_modify_pinned_component(self): """ @@ -420,7 +442,7 @@ def test_modify_pinned_component(self): unit.refresh_from_db() # Reloading the unit is necessary, or 'unit.versioning' will be outdated self.component_1.refresh_from_db() assert unit.versioning.has_unpublished_changes is False # Shallow check - assert authoring_api.contains_unpublished_changes(unit) is False # Deep check + assert authoring_api.contains_unpublished_changes(unit.pk) is False # Deep check assert self.component_1.versioning.has_unpublished_changes is True # Neither the draft nor the published version of the unit is affected @@ -496,14 +518,14 @@ def test_publishing_shared_component(self): unit1 = self.create_unit_with_components([c1, c2, c3], title="Unit 1", key="unit:1") unit2 = self.create_unit_with_components([c2, c4, c5], title="Unit 2", key="unit:2") authoring_api.publish_all_drafts(self.learning_package.id) - assert authoring_api.contains_unpublished_changes(unit1) is False - assert authoring_api.contains_unpublished_changes(unit2) is False + assert authoring_api.contains_unpublished_changes(unit1.pk) is False + assert authoring_api.contains_unpublished_changes(unit2.pk) is False # 2️⃣ Then the author edits C2 inside of Unit 1 making C2v2. c2_v2 = self.modify_component(c2, title="C2 version 2") # This makes U1 and U2 both show up as Units that CONTAIN unpublished changes, because they share the component. - assert authoring_api.contains_unpublished_changes(unit1) - assert authoring_api.contains_unpublished_changes(unit2) + assert authoring_api.contains_unpublished_changes(unit1.pk) + assert authoring_api.contains_unpublished_changes(unit2.pk) # (But the units themselves are unchanged:) unit1.refresh_from_db() unit2.refresh_from_db() @@ -539,8 +561,8 @@ def test_publishing_shared_component(self): ] # Result: Unit 2 CONTAINS unpublished changes because of the modified C5. Unit 1 doesn't contain unpub changes. - assert authoring_api.contains_unpublished_changes(unit1) is False - assert authoring_api.contains_unpublished_changes(unit2) + assert authoring_api.contains_unpublished_changes(unit1.pk) is False + assert authoring_api.contains_unpublished_changes(unit2.pk) # 5️⃣ Publish component C5, which should be the only thing unpublished in the learning package self.publish_component(c5) @@ -550,7 +572,7 @@ def test_publishing_shared_component(self): Entry(c4_v1), # still original version of C4 (it was never modified) Entry(c5_v2), # new published version of C5 ] - assert authoring_api.contains_unpublished_changes(unit2) is False + assert authoring_api.contains_unpublished_changes(unit2.pk) is False def test_query_count_of_contains_unpublished_changes(self): """ @@ -570,12 +592,12 @@ def test_query_count_of_contains_unpublished_changes(self): authoring_api.publish_all_drafts(self.learning_package.id) unit.refresh_from_db() with self.assertNumQueries(2): - assert authoring_api.contains_unpublished_changes(unit) is False + assert authoring_api.contains_unpublished_changes(unit.pk) is False # Modify the most recently created component: self.modify_component(component, title="Modified Component") with self.assertNumQueries(2): - assert authoring_api.contains_unpublished_changes(unit) is True + assert authoring_api.contains_unpublished_changes(unit.pk) is True def test_metadata_change_doesnt_create_entity_list(self): """ @@ -585,14 +607,14 @@ def test_metadata_change_doesnt_create_entity_list(self): """ unit = self.create_unit_with_components([self.component_1, self.component_2_v1]) - orig_version_num = unit.container.versioning.draft.version_num - orig_entity_list_id = unit.container.versioning.draft.entity_list.pk + orig_version_num = unit.versioning.draft.version_num + orig_entity_list_id = unit.versioning.draft.entity_list.pk authoring_api.create_next_unit_version(unit, title="New Title", created=self.now) unit.refresh_from_db() - new_version_num = unit.container.versioning.draft.version_num - new_entity_list_id = unit.container.versioning.draft.entity_list.pk + new_version_num = unit.versioning.draft.version_num + new_entity_list_id = unit.versioning.draft.entity_list.pk assert new_version_num > orig_version_num assert new_entity_list_id == orig_entity_list_id @@ -636,7 +658,7 @@ def test_removing_component(self): ] unit.refresh_from_db() assert unit.versioning.has_unpublished_changes # The unit itself and its component list have change - assert authoring_api.contains_unpublished_changes(unit) + assert authoring_api.contains_unpublished_changes(unit.pk) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), @@ -650,7 +672,7 @@ def test_removing_component(self): # a footgun? We could avoid this if get_entities_in_published_container() took only an ID instead of an object, # but that would involve additional database lookup(s). unit.refresh_from_db() - assert authoring_api.contains_unpublished_changes(unit) is False + assert authoring_api.contains_unpublished_changes(unit.pk) is False assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), ] @@ -672,7 +694,7 @@ def test_soft_deleting_component(self): # reverted? ] assert unit.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed - assert authoring_api.contains_unpublished_changes(unit) # But it CONTAINS an unpublished change (a deletion) + assert authoring_api.contains_unpublished_changes(unit.pk) # But it CONTAINS an unpublished change (a deletion) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), @@ -681,7 +703,7 @@ def test_soft_deleting_component(self): # But when we publish the deletion, the published version is affected: authoring_api.publish_all_drafts(self.learning_package.id) - assert authoring_api.contains_unpublished_changes(unit) is False + assert authoring_api.contains_unpublished_changes(unit.pk) is False assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), ] @@ -706,7 +728,7 @@ def test_soft_deleting_and_removing_component(self): Entry(self.component_1_v1), ] assert unit.versioning.has_unpublished_changes is True - assert authoring_api.contains_unpublished_changes(unit) + assert authoring_api.contains_unpublished_changes(unit.pk) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), @@ -715,7 +737,7 @@ def test_soft_deleting_and_removing_component(self): # But when we publish the deletion, the published version is affected: authoring_api.publish_all_drafts(self.learning_package.id) - assert authoring_api.contains_unpublished_changes(unit) is False + assert authoring_api.contains_unpublished_changes(unit.pk) is False assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1), ] @@ -734,7 +756,7 @@ def test_soft_deleting_pinned_component(self): Entry(self.component_2_v1, pinned=True), ] assert unit.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed - assert authoring_api.contains_unpublished_changes(unit) is False # nor does it contain changes + assert authoring_api.contains_unpublished_changes(unit.pk) is False # nor does it contain changes # The published version of the unit is also not affected: assert authoring_api.get_components_in_published_unit(unit) == [ Entry(self.component_1_v1, pinned=True), @@ -872,10 +894,7 @@ def test_units_containing(self): # No need to publish anything as the get_containers_with_entity() API only considers drafts (for now). with self.assertNumQueries(1): - result = [ - c.unit for c in - authoring_api.get_containers_with_entity(self.component_1.pk).select_related("unit") - ] + result = list(authoring_api.get_containers_with_entity(self.component_1.pk)) assert result == [ unit1_1pinned, unit2_1pinned_v2, @@ -886,10 +905,7 @@ def test_units_containing(self): # about pinned uses anyways (they would be unaffected by a delete). with self.assertNumQueries(1): - result2 = [ - c.unit for c in - authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True).select_related("unit") - ] + result2 = list(authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True)) assert result2 == [unit4_unpinned] # Tests TODO: From 13004f9206e7d1903362f1cf074e3e45284c43dc Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 08:41:39 -0700 Subject: [PATCH 2/7] refactor: use multi-table inheritance --- .../apps/authoring/publishing/api.py | 6 +- .../publishing/migrations/0003_containers.py | 5 +- .../authoring/publishing/models/container.py | 91 ------------------- openedx_learning/apps/authoring/units/api.py | 65 +++++++------ .../units/migrations/0001_initial.py | 15 ++- .../apps/authoring/units/models.py | 26 ++++-- .../apps/authoring/units/test_api.py | 34 ++----- 7 files changed, 70 insertions(+), 172 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 5fece0ac..3d96c095 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -574,7 +574,6 @@ def get_published_version_as_of(entity_id: int, publish_log_id: int) -> Publisha def create_container( learning_package_id: int, key: str, - container_type: str, created: datetime, created_by: int | None, ) -> Container: @@ -591,14 +590,12 @@ def create_container( Returns: The newly created container. """ - assert container_type # Shouldn't be empty/none with atomic(): publishable_entity = create_publishable_entity( learning_package_id, key, created, created_by ) container = Container.objects.create( publishable_entity=publishable_entity, - container_type=container_type, ) return container @@ -787,7 +784,6 @@ def create_container_and_version( learning_package_id: int, key: str, *, - container_type: str, created: datetime, created_by: int | None, title: str, @@ -811,7 +807,7 @@ def create_container_and_version( The newly created container version. """ with atomic(): - container = create_container(learning_package_id, key, container_type, created, created_by) + container = create_container(learning_package_id, key, created, created_by) container_version = create_container_version( container.publishable_entity.pk, 1, diff --git a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py index 8048017e..1f759144 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0003_containers.py @@ -1,10 +1,8 @@ -# Generated by Django 4.2.19 on 2025-03-10 18:03 +# Generated by Django 4.2.19 on 2025-03-11 04:10 import django.db.models.deletion from django.db import migrations, models -import openedx_learning.lib.fields - class Migration(migrations.Migration): @@ -17,7 +15,6 @@ class Migration(migrations.Migration): name='Container', fields=[ ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('container_type', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/publishing/models/container.py b/openedx_learning/apps/authoring/publishing/models/container.py index 0e4d9937..4a0df08a 100644 --- a/openedx_learning/apps/authoring/publishing/models/container.py +++ b/openedx_learning/apps/authoring/publishing/models/container.py @@ -1,54 +1,11 @@ """ Container and ContainerVersion models """ -from typing import ClassVar, Self, TypeVar - -from django.core.exceptions import ValidationError from django.db import models -from openedx_learning.lib.fields import case_sensitive_char_field -from openedx_learning.lib.managers import WithRelationsManager - from ..model_mixins.publishable_entity import PublishableEntityMixin, PublishableEntityVersionMixin from .entity_list import EntityList -M = TypeVar('M', bound="Container") - - -class ContainerManager(WithRelationsManager[M]): - """ - A custom manager used for Container and its subclasses. - """ - def __init__(self): - """ - Initialize the manager for Container / a Container subclass - """ - super().__init__( - # Select these related entities by default: - "publishable_entity", - "publishable_entity__published", - "publishable_entity__draft", - ) - - def get_queryset(self) -> models.QuerySet: - """ - Apply filter() and select_related() to all querysets. - """ - qs = super().get_queryset() - if self.model.CONTAINER_TYPE: - qs = qs.filter(container_type=self.model.CONTAINER_TYPE) - return qs - - def create(self, **kwargs) -> M: - """ - Apply the values from our filter when creating new instances. - """ - if self.model.CONTAINER_TYPE: - # Don't allow creating via a subclass, like Unit.objects.create(). - # Instead use create_container() which calls Container.objects.create(..., container_type=...) - raise ValidationError("Container instances should only be created via APIs like create_container()") - return super().create(**kwargs) - class Container(PublishableEntityMixin): """ @@ -65,40 +22,6 @@ class Container(PublishableEntityMixin): PublishLog and Containers that were affected in a publish because their child elements were published. """ - # Subclasses (django proxy classes) should override this - CONTAINER_TYPE = "" - - objects: ClassVar[ContainerManager[Self]] = ContainerManager() # type: ignore[assignment] - - container_type = case_sensitive_char_field(max_length=500) - - def save(self, *args, **kwargs): - if not self.container_type: - raise ValidationError("Container instances should only be created via APIs like create_container()") - return super().save(*args, **kwargs) - - def clean(self): - """ - Validate this container subclass - """ - if self.container_type and self.CONTAINER_TYPE: - if self.container_type != self.CONTAINER_TYPE: - raise ValidationError("container type field mismatch with model.") - super().clean() - - @classmethod - def cast_from(cls, instance: "Container") -> Self: - """ - Create a new copy of a Container object, with a different subclass - """ - assert instance.container_type == cls.CONTAINER_TYPE - new_instance = cls( - pk=instance.pk, - container_type=instance.container_type, - ) - # Copy Django's internal cache of related objects - new_instance._state.fields_cache.update(instance._state.fields_cache) # pylint: disable=protected-access - return new_instance class ContainerVersion(PublishableEntityVersionMixin): @@ -135,17 +58,3 @@ class ContainerVersion(PublishableEntityVersionMixin): null=False, related_name="container_versions", ) - - @classmethod - def cast_from(cls, instance: "ContainerVersion") -> Self: - """ - Create a new copy of a Container object, with a different subclass - """ - new_instance = cls( - pk=instance.pk, - container_id=instance.container_id, - entity_list_id=instance.entity_list_id, - ) - # Copy Django's internal cache of related objects - new_instance._state.fields_cache.update(instance._state.fields_cache) # pylint: disable=protected-access - return new_instance diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 57700ff2..5de60f30 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -41,16 +41,14 @@ def create_unit( created: The creation date. created_by: The user who created the unit. """ - container = publishing_api.create_container( - learning_package_id, - key, - Unit.CONTAINER_TYPE, - created, - created_by, - ) - return Unit.cast_from(container) - # Or, more robust but using additional queries: - # return Unit.objects.select_related(None).get(pk=container.pk) + with atomic(): + container = publishing_api.create_container( + learning_package_id, + key, + created, + created_by, + ) + return Unit.objects.create(container_id=container.pk) def create_unit_version( @@ -79,17 +77,20 @@ def create_unit_version( created: The creation date. created_by: The user who created the unit. """ - container_version = publishing_api.create_container_version( - unit.pk, - version_num, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - # return UnitVersion.objects.get(pk=container_version.pk) - return UnitVersion.cast_from(container_version) + with atomic(): + container_version = publishing_api.create_container_version( + unit.pk, + version_num, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + ) + # unit_version = UnitVersion(container_version=container_version) # 🛑 This has a bug :/ + unit_version = UnitVersion(container_version_id=container_version.pk) + unit_version.save_base(raw=True) # 🛑 private method, but calling just .save() or .create() causes a bug :/ + return unit_version def _pub_entities_for_components( @@ -140,15 +141,19 @@ def create_next_unit_version( created_by: The user who created the unit. """ publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components) - container_version = publishing_api.create_next_container_version( - unit.pk, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - return UnitVersion.cast_from(container_version) + with atomic(): + container_version = publishing_api.create_next_container_version( + unit.pk, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + ) + # unit_version = UnitVersion(container_version=container_version) # 🛑 This has a bug :/ + unit_version = UnitVersion(container_version_id=container_version.pk) + unit_version.save_base(raw=True) # 🛑 private method, but calling just .save() or .create() causes a bug :/ + return unit_version def create_unit_and_version( diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py index f3c8a477..52a074f6 100644 --- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py @@ -1,6 +1,7 @@ -# Generated by Django 4.2.19 on 2025-03-10 18:18 +# Generated by Django 4.2.19 on 2025-03-11 04:31 -from django.db import migrations +from django.db import migrations, models +import django.db.models.deletion class Migration(migrations.Migration): @@ -15,22 +16,20 @@ class Migration(migrations.Migration): migrations.CreateModel( name='Unit', fields=[ + ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.container')), ], options={ - 'proxy': True, - 'indexes': [], - 'constraints': [], + 'abstract': False, }, bases=('oel_publishing.container',), ), migrations.CreateModel( name='UnitVersion', fields=[ + ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.containerversion')), ], options={ - 'proxy': True, - 'indexes': [], - 'constraints': [], + 'abstract': False, }, bases=('oel_publishing.containerversion',), ), diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 60ff982c..425b1738 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -1,6 +1,7 @@ """ Models that implement units """ +from django.db import models from ..publishing.models import Container, ContainerVersion @@ -14,20 +15,25 @@ class Unit(Container): """ A Unit is Container, which is a PublishableEntity. """ - CONTAINER_TYPE = "unit" + container = models.OneToOneField( + Container, + on_delete=models.CASCADE, + parent_link=True, + primary_key=True, + ) - class Meta: - proxy = True + @property + def versioning(self): + return self.container.versioning class UnitVersion(ContainerVersion): """ A UnitVersion is a ContainerVersion, which is a PublishableEntityVersion. """ - - @property - def unit(self): - return Unit.objects.get(pk=self.container_id) - - class Meta: - proxy = True + container_version = models.OneToOneField( + ContainerVersion, + on_delete=models.CASCADE, + parent_link=True, + primary_key=True, + ) diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index 0a3c7ce7..4514d239 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -90,26 +90,6 @@ def test_get_unit(self): with self.assertNumQueries(0): assert result.versioning.has_unpublished_changes - def test_get_unit_non_unit(self): - """ - Test that get_unit() cannot retrieve other container types - """ - not_unit, _version = authoring_api.create_container_and_version( - self.learning_package.id, - key="foobar", - container_type="NOT a unit", # <-- the important part - created=self.now, - created_by=None, - title="Testing", - publishable_entities_pks=[], - entity_version_pks=[] - ) - # This generic method will work: - authoring_api.get_container(not_unit.pk) - # But the unit method will not: - with self.assertRaises(authoring_models.Unit.DoesNotExist): - authoring_api.get_unit(not_unit.pk) - def test_get_container(self): """ Test get_container() @@ -127,9 +107,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(18): + with self.assertNumQueries(26): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(21): + 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") @@ -894,7 +874,10 @@ def test_units_containing(self): # No need to publish anything as the get_containers_with_entity() API only considers drafts (for now). with self.assertNumQueries(1): - result = list(authoring_api.get_containers_with_entity(self.component_1.pk)) + result = [ + c.unit for c in + authoring_api.get_containers_with_entity(self.component_1.pk).select_related("unit") + ] assert result == [ unit1_1pinned, unit2_1pinned_v2, @@ -905,7 +888,10 @@ def test_units_containing(self): # about pinned uses anyways (they would be unaffected by a delete). with self.assertNumQueries(1): - result2 = list(authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True)) + result2 = [ + c.unit for c in + authoring_api.get_containers_with_entity(self.component_1.pk, ignore_pinned=True).select_related("unit") + ] assert result2 == [unit4_unpinned] # Tests TODO: From 776e471d67fbde48b2b9cc1ee06fa0e57d4781ee Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 15:01:54 -0700 Subject: [PATCH 3/7] fix: create subclass models properly --- .../apps/authoring/publishing/api.py | 11 +++- openedx_learning/apps/authoring/units/api.py | 60 ++++++++----------- .../apps/authoring/units/models.py | 4 -- .../apps/authoring/units/test_api.py | 6 +- 4 files changed, 37 insertions(+), 44 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 3d96c095..22637a17 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -576,6 +576,7 @@ def create_container( key: str, created: datetime, created_by: int | None, + container_model: type[Container] = Container, ) -> Container: """ [ 🛑 UNSTABLE ] @@ -594,7 +595,7 @@ def create_container( publishable_entity = create_publishable_entity( learning_package_id, key, created, created_by ) - container = Container.objects.create( + container = container_model.objects.create( publishable_entity=publishable_entity, ) return container @@ -657,6 +658,7 @@ def create_container_version( entity_version_pks: list[int | None] | None, created: datetime, created_by: int | None, + container_version_model: type[ContainerVersion] = ContainerVersion, ) -> ContainerVersion: """ [ 🛑 UNSTABLE ] @@ -670,6 +672,7 @@ def create_container_version( entity_version_pks: The IDs of the versions to pin to, if pinning is desired. created: The date and time the container version was created. created_by: The ID of the user who created the container version. + container_version_model: The subclass of ContainerVersion to use, if applicable. Returns: The newly created container version. @@ -701,7 +704,7 @@ def create_container_version( created=created, created_by=created_by, ) - container_version = ContainerVersion.objects.create( + container_version = container_version_model.objects.create( publishable_entity_version=publishable_entity_version, container_id=container_pk, entity_list=entity_list, @@ -718,6 +721,7 @@ def create_next_container_version( entity_version_pks: list[int | None] | None, created: datetime, created_by: int | None, + container_version_model: type[ContainerVersion] = ContainerVersion, ) -> ContainerVersion: """ [ 🛑 UNSTABLE ] @@ -737,6 +741,7 @@ def create_next_container_version( entity_version_pks: The IDs of the versions to pin to, if pinning is desired. created: The date and time the container version was created. created_by: The ID of the user who created the container version. + container_version_model: The subclass of ContainerVersion to use, if applicable. Returns: The newly created container version. @@ -771,7 +776,7 @@ def create_next_container_version( created=created, created_by=created_by, ) - next_container_version = ContainerVersion.objects.create( + next_container_version = container_version_model.objects.create( publishable_entity_version=publishable_entity_version, container_id=container_pk, entity_list=next_entity_list, diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 5de60f30..d007a57f 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -41,14 +41,13 @@ def create_unit( created: The creation date. created_by: The user who created the unit. """ - with atomic(): - container = publishing_api.create_container( - learning_package_id, - key, - created, - created_by, - ) - return Unit.objects.create(container_id=container.pk) + return publishing_api.create_container( + learning_package_id, + key, + created, + created_by, + container_model=Unit, + ) def create_unit_version( @@ -77,20 +76,16 @@ def create_unit_version( created: The creation date. created_by: The user who created the unit. """ - with atomic(): - container_version = publishing_api.create_container_version( - unit.pk, - version_num, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - # unit_version = UnitVersion(container_version=container_version) # 🛑 This has a bug :/ - unit_version = UnitVersion(container_version_id=container_version.pk) - unit_version.save_base(raw=True) # 🛑 private method, but calling just .save() or .create() causes a bug :/ - return unit_version + return publishing_api.create_container_version( + unit.pk, + version_num, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + container_version_model=UnitVersion, + ) def _pub_entities_for_components( @@ -141,18 +136,15 @@ def create_next_unit_version( created_by: The user who created the unit. """ publishable_entities_pks, entity_version_pks = _pub_entities_for_components(components) - with atomic(): - container_version = publishing_api.create_next_container_version( - unit.pk, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - # unit_version = UnitVersion(container_version=container_version) # 🛑 This has a bug :/ - unit_version = UnitVersion(container_version_id=container_version.pk) - unit_version.save_base(raw=True) # 🛑 private method, but calling just .save() or .create() causes a bug :/ + unit_version = publishing_api.create_next_container_version( + unit.pk, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + container_version_model=UnitVersion, + ) return unit_version diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 425b1738..254d42e1 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -22,10 +22,6 @@ class Unit(Container): primary_key=True, ) - @property - def versioning(self): - return self.container.versioning - class UnitVersion(ContainerVersion): """ diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index 4514d239..320cb3c3 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -97,7 +97,7 @@ def test_get_container(self): unit = self.create_unit_with_components([self.component_1, self.component_2]) with self.assertNumQueries(1): result = authoring_api.get_container(unit.pk) - assert result == unit + assert result == unit.container # Versioning data should be pre-loaded via select_related() with self.assertNumQueries(0): assert result.versioning.has_unpublished_changes @@ -107,9 +107,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(26): + with self.assertNumQueries(22): _empty_unit = self.create_unit_with_components([]) - with self.assertNumQueries(29): + with self.assertNumQueries(25): # And try with a non-empty unit: self.create_unit_with_components([self.component_1, self.component_2_v1], key="u2") From ac32fefe51ecbb499add555fc22ea8416ee385dc Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 15:58:09 -0700 Subject: [PATCH 4/7] feat: improve versioning helper to return the correct class --- .../model_mixins/publishable_entity.py | 20 +++++++++++++++++-- .../apps/authoring/units/models.py | 9 +++++++++ .../apps/authoring/units/test_api.py | 17 +++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/model_mixins/publishable_entity.py b/openedx_learning/apps/authoring/publishing/model_mixins/publishable_entity.py index 49aaac93..84360310 100644 --- a/openedx_learning/apps/authoring/publishing/model_mixins/publishable_entity.py +++ b/openedx_learning/apps/authoring/publishing/model_mixins/publishable_entity.py @@ -135,11 +135,24 @@ def __init__(self, content_obj): field_to_pev = self.content_version_model_cls._meta.get_field( "publishable_entity_version" ) - # Now that we know the field that leads to PublishableEntityVersion, # get the reverse related field name so that we can use that later. self.related_name = field_to_pev.related_query_name() + if field_to_pev.model != self.content_version_model_cls: + # In the case of multi-table inheritance and mixins, this can get tricky. + # Example: + # content_version_model_cls is UnitVersion, which is a subclass of ContainerVersion + # This versioning helper can be accessed via unit_version.versioning (should return UnitVersion) or + # via container_version.versioning (should return ContainerVersion) + intermediate_model = field_to_pev.model # example: ContainerVersion + # This is the field on the subclass (e.g. UnitVersion) that gets + # the intermediate (e.g. ContainerVersion). Example: "UnitVersion.container_version" (1:1 foreign key) + field_to_intermediate = self.content_version_model_cls._meta.get_ancestor_link(intermediate_model) + if field_to_intermediate: + # Example: self.related_name = "containerversion.unitversion" + self.related_name = self.related_name + "." + field_to_intermediate.related_query_name() + def _content_obj_version(self, pub_ent_version: PublishableEntityVersion | None): """ PublishableEntityVersion -> Content object version @@ -149,7 +162,10 @@ def _content_obj_version(self, pub_ent_version: PublishableEntityVersion | None) """ if pub_ent_version is None: return None - return getattr(pub_ent_version, self.related_name) + obj = pub_ent_version + for field_name in self.related_name.split("."): + obj = getattr(obj, field_name) + return obj @property def draft(self): diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 254d42e1..d74aa883 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -33,3 +33,12 @@ class UnitVersion(ContainerVersion): parent_link=True, primary_key=True, ) + + @property + def unit(self): + """ Convenience accessor to the Unit this version is associated with """ + return self.container_version.container.unit # pylint: disable=no-member + + # Note: the 'publishable_entity_version' field is inherited and will appear on this model, but does not exist + # in the underlying database table. It only exists in the ContainerVersion table. + # You can verify this by running 'python manage.py sqlmigrate oel_units 0001_initial' diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index 320cb3c3..52830206 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -102,6 +102,21 @@ def test_get_container(self): with self.assertNumQueries(0): assert result.versioning.has_unpublished_changes + def test_unit_container_versioning(self): + """ + Test that the .versioning helper of a Unit returns a UnitVersion, and + same for the generic Container equivalent. + """ + unit = self.create_unit_with_components([self.component_1, self.component_2]) + container = unit.container + container_version = container.versioning.draft + assert isinstance(container_version, authoring_models.ContainerVersion) + unit_version = unit.versioning.draft + assert isinstance(unit_version, authoring_models.UnitVersion) + assert unit_version.container_version == container_version + assert unit_version.container_version.container == container + assert unit_version.unit == unit + def test_create_unit_queries(self): """ Test how many database queries are required to create a unit @@ -145,7 +160,7 @@ def test_create_unit_with_invalid_children(self): ) # Check that a new version was not created: unit.refresh_from_db() - assert authoring_api.get_container(unit.pk).versioning.draft == unit_version + assert authoring_api.get_unit(unit.pk).versioning.draft == unit_version assert unit.versioning.draft == unit_version def test_adding_external_components(self): From fca9b3ffd7c8e96d07acd4c69d5f3d9321f596b5 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 16:29:09 -0700 Subject: [PATCH 5/7] refactor: make container API methods generic, remove unused function --- .../apps/authoring/publishing/api.py | 59 ++++--------------- .../units/migrations/0001_initial.py | 2 +- 2 files changed, 13 insertions(+), 48 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 22637a17..e5f5df1d 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -8,6 +8,7 @@ from dataclasses import dataclass from datetime import datetime, timezone +from typing import TypeVar from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet @@ -28,6 +29,9 @@ PublishLogRecord, ) +ContainerModel = TypeVar('ContainerModel', bound=Container) +ContainerVersionModel = TypeVar('ContainerVersionModel', bound=ContainerVersion) + # The public API that will be re-exported by openedx_learning.apps.authoring.api # is listed in the __all__ entries below. Internal helper functions that are # private to this module should start with an underscore. If a function does not @@ -61,7 +65,6 @@ "create_container", "create_container_version", "create_next_container_version", - "create_container_and_version", "get_container", "ContainerEntityListEntry", "get_entities_in_draft_container", @@ -576,8 +579,9 @@ def create_container( key: str, created: datetime, created_by: int | None, - container_model: type[Container] = Container, -) -> Container: + # The types on the following line are correct, but mypy will complain - https://github.com/python/mypy/issues/3737 + container_model: type[ContainerModel] = Container, # type: ignore[assignment] +) -> ContainerModel: """ [ 🛑 UNSTABLE ] Create a new container. @@ -587,6 +591,7 @@ def create_container( key: The key of the container. created: The date and time the container was created. created_by: The ID of the user who created the container + container_model: The subclass of Container to use, if applicable Returns: The newly created container. @@ -658,8 +663,8 @@ def create_container_version( entity_version_pks: list[int | None] | None, created: datetime, created_by: int | None, - container_version_model: type[ContainerVersion] = ContainerVersion, -) -> ContainerVersion: + container_version_model: type[ContainerVersionModel] = ContainerVersion, # type: ignore[assignment] +) -> ContainerVersionModel: """ [ 🛑 UNSTABLE ] Create a new container version. @@ -721,8 +726,8 @@ def create_next_container_version( entity_version_pks: list[int | None] | None, created: datetime, created_by: int | None, - container_version_model: type[ContainerVersion] = ContainerVersion, -) -> ContainerVersion: + container_version_model: type[ContainerVersionModel] = ContainerVersion, # type: ignore[assignment] +) -> ContainerVersionModel: """ [ 🛑 UNSTABLE ] Create the next version of a container. A new version of the container is created @@ -785,46 +790,6 @@ def create_next_container_version( return next_container_version -def create_container_and_version( - learning_package_id: int, - key: str, - *, - created: datetime, - created_by: int | None, - title: str, - publishable_entities_pks: list[int], - entity_version_pks: list[int | None], -) -> tuple[Container, ContainerVersion]: - """ - [ 🛑 UNSTABLE ] - Create a new container and its first version. - - Args: - learning_package_id: The ID of the learning package that contains the container. - key: The key of the container. - created: The date and time the container was created. - created_by: The ID of the user who created the container. - version_num: The version number of the container. - title: The title of the container. - members_pk: The IDs of the members of the container. - - Returns: - The newly created container version. - """ - with atomic(): - container = create_container(learning_package_id, key, created, created_by) - container_version = create_container_version( - container.publishable_entity.pk, - 1, - title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, - created=created, - created_by=created_by, - ) - return (container, container_version) - - def get_container(pk: int) -> Container: """ [ 🛑 UNSTABLE ] diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py index 52a074f6..52a5b4fb 100644 --- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py @@ -1,7 +1,7 @@ # Generated by Django 4.2.19 on 2025-03-11 04:31 -from django.db import migrations, models import django.db.models.deletion +from django.db import migrations, models class Migration(migrations.Migration): From 08ed8a032a47848829f72ea5ea473b8eda61e06e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 17:08:29 -0700 Subject: [PATCH 6/7] feat: sprinkle in a little validation, at least for dev --- openedx_learning/apps/authoring/publishing/api.py | 3 +++ .../apps/authoring/publishing/models/container.py | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index e5f5df1d..d73dfbff 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -596,6 +596,7 @@ def create_container( Returns: The newly created container. """ + assert issubclass(container_model, Container) with atomic(): publishable_entity = create_publishable_entity( learning_package_id, key, created, created_by @@ -682,6 +683,7 @@ def create_container_version( Returns: The newly created container version. """ + assert issubclass(container_version_model, ContainerVersion) with atomic(savepoint=False): container = Container.objects.select_related("publishable_entity").get(pk=container_pk) entity = container.publishable_entity @@ -751,6 +753,7 @@ def create_next_container_version( Returns: The newly created container version. """ + assert issubclass(container_version_model, ContainerVersion) with atomic(): container = Container.objects.select_related("publishable_entity").get(pk=container_pk) entity = container.publishable_entity diff --git a/openedx_learning/apps/authoring/publishing/models/container.py b/openedx_learning/apps/authoring/publishing/models/container.py index 4a0df08a..04152f71 100644 --- a/openedx_learning/apps/authoring/publishing/models/container.py +++ b/openedx_learning/apps/authoring/publishing/models/container.py @@ -1,6 +1,7 @@ """ Container and ContainerVersion models """ +from django.core.exceptions import ValidationError from django.db import models from ..model_mixins.publishable_entity import PublishableEntityMixin, PublishableEntityVersionMixin @@ -58,3 +59,12 @@ class ContainerVersion(PublishableEntityVersionMixin): null=False, related_name="container_versions", ) + + def clean(self): + """ + Validate this model before saving. Not called normally, but will be + called if anything is edited via a ModelForm like the Django admin. + """ + super().clean() + if self.container_id != self.publishable_entity_version.entity.container.pk: # pylint: disable=no-member + raise ValidationError("Inconsistent foreign keys to Container") From d8954f5bee827254b71fcca0a30b69c9ac5771b7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 11 Mar 2025 17:14:24 -0700 Subject: [PATCH 7/7] docs: improve docstrings --- openedx_learning/apps/authoring/units/models.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index d74aa883..0c525584 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -13,7 +13,10 @@ class Unit(Container): """ - A Unit is Container, which is a PublishableEntity. + A Unit is type of Container that holds Components. + + Via Container and its PublishableEntityMixin, Units are also publishable + entities and can be added to other containers. """ container = models.OneToOneField( Container, @@ -25,7 +28,10 @@ class Unit(Container): class UnitVersion(ContainerVersion): """ - A UnitVersion is a ContainerVersion, which is a PublishableEntityVersion. + A UnitVersion is a specific version of a Unit. + + Via ContainerVersion and its EntityList, it defines the list of Components + in this version of the Unit. """ container_version = models.OneToOneField( ContainerVersion,